date:20151014

On 2015-10-14 17:46:25 +0300, Amir Rohan wrote:
> On 10/14/2015 05:35 PM, Andres Freund wrote:
> > Then your argument about the CF process doesn't seem to make sense.

> Why? I ask again, what do you mean by "separate process"?

Not going through the CF and normal release process.

> either it's in core (and follows its processes) or it isn't. But you
> can't say you don't want it in core but that you also don't
> want it to follow a "separate process".

Oh for crying out loud. You write:

> 4) You can't easily extend the checks performed, without forking
> postgres or going through the (lengthy, rigorous) cf process.

and

> > I don't think we as a community want to do that without review
> > mechanisms in place, and I personally don't think we want to add
> > separate processes for this.

> That's what "contribute" means in my book.

I don't see how those two statements don't conflict.


-- 
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

On 2015-10-14 18:53:14 +0300, Shay Rojansky wrote:
> However, the new situation where some versions of PG allow this parameter
> while others bomb when seeing it. Specifically, Npgsql sends
> ssl_renegotiation_limit=0 in the startup packet to completely disable
> renegotiation. At this early stage it doesn't know yet whether the database
> it's connecting to is PG 9.5 or earlier.

I find it a rather debatable practice to send such a parameter
unconditionally. Why are you sending it before the connection has even
been established?

> Is there any chance you'd consider allowing ssl_renegotiation_limit in PG
> 9.5, without it having any effect (I think that's the current behavior for
> recent 9.4, 9.3, right)?

No, you can actually enable renegotiation in those versions, it's just a
changed default value.

> It may be a good idea to only allow this parameter to be set to zero,
> raising an error otherwise.

-0.1 from me.

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] Proposal: pg_confcheck - syntactic & semantic validation of postgresql configuration files

On 10/14/2015 05:55 PM, Andres Freund wrote:
> On 2015-10-14 17:46:25 +0300, Amir Rohan wrote:
>> On 10/14/2015 05:35 PM, Andres Freund wrote:
>>> Then your argument about the CF process doesn't seem to make sense.
> 
>> Why? I ask again, what do you mean by "separate process"?
> 
> Not going through the CF and normal release process.
> 
>> either it's in core (and follows its processes) or it isn't. But you
>> can't say you don't want it in core but that you also don't
>> want it to follow a "separate process".
> 
> Oh for crying out loud. You write:
> 

Andres, I'm not here looking for ways to quibble with you.
So, please "assume good faith".

>> 4) You can't easily extend the checks performed, without forking
>> postgres or going through the (lengthy, rigorous) cf process.
> 
> and
> 
>>> I don't think we as a community want to do that without review
>>> mechanisms in place, and I personally don't think we want to add
>>> separate processes for this.
> 
>> That's what "contribute" means in my book.
> 
> I don't see how those two statements don't conflict.
> 

Right.

I was saying that "contribute" always implies review before acceptance,
responding to the first half of your sentence. The second half
assumes it makes sense to discuss "review process" as a separate issue
from inclusion in core. It does not make sense, and I said so.

If you have a bone to pick with my comment about CF review being
lengthy, the points was that an independent tool can move more
quickly to accept submissions because:
1. there's less at stake
2. if it's written in a higher level language, enhancements
are easier.

Those don't hold when adding another check involves changes to the
`postgres` binary.

Fair?

Amir

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

[HACKERS] Allow ssl_renegotiation_limit in PG 9.5

2015-10-14 Thread Shay Rojansky

Hi hackers.

I noticed ssl_renegotiation_limit has been removed in PostgreSQL 9.5, good
riddance...

However, the new situation where some versions of PG allow this parameter
while others bomb when seeing it. Specifically, Npgsql sends
ssl_renegotiation_limit=0 in the startup packet to completely disable
renegotiation. At this early stage it doesn't know yet whether the database
it's connecting to is PG 9.5 or earlier.

Is there any chance you'd consider allowing ssl_renegotiation_limit in PG
9.5, without it having any effect (I think that's the current behavior for
recent 9.4, 9.3, right)? It may be a good idea to only allow this parameter
to be set to zero, raising an error otherwise.

The alternative would be to force users to specify in advance whether the
database they're connecting to supports this parameter, or to send it after
the startup packet which complicates things etc.

Thanks,

Shay

Re: [HACKERS] Proposal: pg_confcheck - syntactic & semantic validation of postgresql configuration files

On 10/14/2015 05:35 PM, Andres Freund wrote:
> On 2015-10-14 16:50:41 +0300, Amir Rohan wrote:
>>> I don't think we as a community want to do that without review
>>> mechanisms in place, and I personally don't think we want to add
>>> separate processes for this.
>>>
>>
>> That's what "contribute" means in my book.
> 
> Then your argument about the CF process doesn't seem to make sense.
> 

Why? I ask again, what do you mean by "separate process"?
either it's in core (and follows its processes) or it isn't. But you
can't say you don't want it in core but that you also don't
want it to follow a "separate process".

I think you're simply saying you're -1 on the whole idea. ok.

Regards,
Amir

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

[HACKERS] OS X El Capitan and DYLD_LIBRARY_PATH

2015-10-14 Thread Peter Eisentraut

The new OS X release 10.11 "El Capitan" has a "security" feature that
prevents passing DYLD_LIBRARY_PATH to child processes.  Somehow, that
variable is stripped from the environment.

Consequently, the current in-tree "make check" test setup will no longer
work, because programs such as initdb and psql that are called several
layers down will no longer be pointed to the right libraries.  If you
run "make install" before "make check", it will work; otherwise
something will either fail because it can't find the libraries, or it
might pick up libraries found somewhere else, with poor outcomes, in my
experience.

The exact specifications of this new behavior aren't clear to me yet.
For example, this C program works just fine:

```
#include 
#include 

int
main()
{
printf("DYLD_LIBRARY_PATH = %s\n", getenv("DYLD_LIBRARY_PATH"));
return 0;
}
```

but this shell script does not:

```
echo "DYLD_LIBRARY_PATH = $DYLD_LIBRARY_PATH"
```

There is breakage all over the Internet if you search for this, but the
full details don't appear to be very clear.

One workaround is to disable System Integrity Protection, effectively
restoring the behavior of the previous OS release.

Or don't upgrade quite yet if you don't want to deal with this at the
moment.


-- 
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: pg_confcheck - syntactic & semantic validation of postgresql configuration files

On 2015-10-14 16:50:41 +0300, Amir Rohan wrote:
> > I don't think we as a community want to do that without review
> > mechanisms in place, and I personally don't think we want to add
> > separate processes for this.
> > 
> 
> That's what "contribute" means in my book.

Then your argument about the CF process doesn't seem to make sense.


-- 
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: pg_confcheck - syntactic & semantic validation of postgresql configuration files

Amir Rohan wrote:

> it does fail the "dependent options" test:
> $ postgres -C "archive_mode"
> on
> $ postgres -C wal_level
> minimal
> 
> no errors, great, let's try it:
> $ pg_ctl restart
> 
> FATAL:  WAL archival cannot be enabled when wal_level is "minimal"

This complaint could be fixed we had a --check-config that runs the
check hook for every variable, I think.  I think that closes all the
syntax checks you want; then your tool only needs to worry about
semantic checks, and can obtain the values by repeated application of -C
 (or, more conveniently, have a new -C mode that takes no args
and prints the names and values for all parameters).

-- 
Á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] Proposal: pg_confcheck - syntactic & semantic validation of postgresql configuration files

On October 14, 2015 7:45:53 PM GMT+02:00, Alvaro Herrera 
 wrote:
>Amir Rohan wrote:
>
>> it does fail the "dependent options" test:
>> $ postgres -C "archive_mode"
>> on
>> $ postgres -C wal_level
>> minimal
>> 
>> no errors, great, let's try it:
>> $ pg_ctl restart
>> 
>> FATAL:  WAL archival cannot be enabled when wal_level is "minimal"
>
>This complaint could be fixed we had a --check-config that runs the
>check hook for every variable, I think.

The problem is that this, and some others, invariant is checked outside the GUC 
framework. Which we should probably change, which IIRC will require some new 
infrastructure.

Andres

--- 
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] Allow ssl_renegotiation_limit in PG 9.5

2015-10-14 Thread Shay Rojansky

Just to give some added reasoning...

As Andres suggested, Npgsql sends ssl_renegotiation_limit=0 because we've
seen renegotiation bugs with the standard .NET SSL implementation (which
Npgsql uses). Seems like everyone has a difficult time with renegotiation.

As Tom suggested, it gets sent in the startup packet so that DISCARD/RESET
ALL resets back to 0 and not to the default 512MB (in older versions).
Npgsql itself issues DISCARD ALL in its connection pool implementation to
reset the connection to its original opened state, and of course users may
want to do it themselves. Having SSL renegotiation accidentally turned on
because a user sent RESET ALL, when the SSL implementation is known to have
issues, is something to be avoided...

Shay

Re: [HACKERS] Can extension build own SGML document?

2015-10-14 Thread Jim Nasby


On 9/15/15 10:13 AM, Tom Lane wrote:

Jim Nasby  writes:

On 9/15/15 8:43 AM, Tom Lane wrote:

AFAICT from a quick look at its documentation, asciidoc can produce
either html or docbook output; so as soon as you want something other
than html output (in particular, PDF), you're back to relying on the
exact same creaky docbook toolchain we use now.  Only with one extra
dependency in front of it.



a2x (http://www.methods.co.nz/asciidoc/a2x.1.html) states that it can
generate "PDF, EPUB, DVI, PS, LaTeX, XHTML (single page or chunked), man
page, HTML Help or plain text formats using asciidoc(1) and other
applications (see REQUISITES section). SOURCE_FILE can also be a DocBook
file with an .xml extension."


AFAICS, for all cases other than HTML output, the "other applications"
are basically the docbook toolchain.


I just started looking at , which seems to be the newer way to handle 
asciidoc. Aside from being a lot faster than a2x/asciidoc, it can 
produce docbook natively. However...



What I expect would be a lot more effort is actually converting all the
SGML to asciidoc. A quick google search doesn't turn up anything promising.


Yeah, the cost of conversion means we're not likely to want to experiment
to see what's better :-(.


If the only concern is handling docbook format (which is what our SGML 
docs produce? Then https://github.com/asciidoctor/asciidoctor-fopub 
might be an option. It's intended for use with asciidoctor, but the 
README does state:


"Using the asciidoctor-fopub project, you can convert any DocBook file 
into a nicely formatted PDF with nothing more than a Java runtime (JVM) 
and development kit (JDK). All the open source software required to 
perform the conversion is automatically fetched from the internet the 
first time you run it."


So maybe it would allow removing some of more problematic parts of the 
toolchain?


Also, if our SGML does produce docbook as an intermediate it might be 
possible to convert that to asciidoc via 
https://github.com/oreillymedia/docbook2asciidoc.

--
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] Allow ssl_renegotiation_limit in PG 9.5

On 2015-10-14 14:19:40 -0300, Alvaro Herrera wrote:
> I think we could continue to have the parameter except that it throws an
> error if you try to set it to something other than 0.

That'll make it hard to ever remove it tho. Not sure if it's worth doing
so for a dubious use of the variable.

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] Proposal: pg_confcheck - syntactic & semantic validation of postgresql configuration files

 IOn 10/14/2015 08:45 PM, Alvaro Herrera wrote:
> Amir Rohan wrote:
> 
>> it does fail the "dependent options" test:
>> $ postgres -C "archive_mode"
>> on
>> $ postgres -C wal_level
>> minimal
>>
>> no errors, great, let's try it:
>> $ pg_ctl restart
>>
>> FATAL:  WAL archival cannot be enabled when wal_level is "minimal"
> 
> This complaint could be fixed we had a --check-config that runs the
> check hook for every variable, I think.  I think that closes all the
> syntax checks you want; then your tool only needs to worry about
> semantic checks, and can obtain the values by repeated application of -C
>  (or, more conveniently, have a new -C mode that takes no args
> and prints the names and values for all parameters).
> 

That sounds reasonable and convenient. It's important to have one
tool that covers everything, so I'd have to call "-C" and parse
the errors, but that seems possible.

If there was a flag which produced something more like the output of the
pg_settings view in a structured format, as well as the errors, that
would be ideal. And possibly useful for other tool building.
Would such a thing be contrary to the pg spirit?

Amir

-- 
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

2015-10-14 Thread Amit Kapila

On Wed, Oct 14, 2015 at 3:29 AM, Robert Haas  wrote:
>
> On Tue, Oct 13, 2015 at 2:45 AM, Amit Kapila 
wrote:
> > Attached is rebased patch for partial seqscan support.
>
> Review comments:
>
>
> - I continue to think GetParallelShmToc is the wrong approach.
> Instead, each time ExecParallelInitializeDSM or
> ExecParallelInitializeDSM calls a nodetype-specific initialized
> function (as described in the previous point), have it pass d->pcxt as
> an argument.  The node can get the toc from there if it needs it.  I
> suppose it could store a pointer to the toc in its scanstate if it
> needs it, but it really shouldn't.  Instead, it should store a pointer
> to, say, the ParallelHeapScanDesc in the scanstate.
>

How will this idea work for worker backend.  Basically in worker
if want something like this to work, toc has to be passed via
QueryDesc to Estate and then we can retrieve ParallelHeapScanDesc
during PartialSeqScan initialization (ExecInitPartialSeqScan).
Do you have something else in mind?

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

Re: [HACKERS] Allow ssl_renegotiation_limit in PG 9.5

On 2015-10-14 13:04:30 -0400, Tom Lane wrote:
> It doesn't seem to me that a connector such as npgsql has any business
> whatsoever fooling with such a parameter, unconditionally or otherwise.

I think in npgsql simply doesn't support renegotiation (IIRC because
it'd have been hard to implement in .net). Which makes it somewhat
reasonable to set it to 0.

-- 
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] Can extension build own SGML document?

Christopher Browne wrote:

> There would be some merit to some remapping to transform "creaky old
> DocBook 4.2" (what we're using) to a newer version, perhaps biased towards
> XML, and have our toolset merge the bits into a big XML (in DocBook 5,
> presumably) file for processing using more modern DocBook tools.

As I recall, last time we discussed this, we found that the process for
XML docbook took 10 times longer to produce the output files.  The XML
toolchain was just too young at the time.  It would be nice to know
whether it has aged well, i.e. has the runtime to build our docs
improved?

-- 
Á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] Can extension build own SGML document?

2015-10-14 Thread Christopher Browne

On 14 October 2015 at 13:04, Jim Nasby  wrote:

> On 9/15/15 10:13 AM, Tom Lane wrote:
>
>> Jim Nasby  writes:
>>
>>> On 9/15/15 8:43 AM, Tom Lane wrote:
>>>
 AFAICT from a quick look at its documentation, asciidoc can produce
 either html or docbook output; so as soon as you want something other
 than html output (in particular, PDF), you're back to relying on the
 exact same creaky docbook toolchain we use now.  Only with one extra
 dependency in front of it.

>>>
>> a2x (http://www.methods.co.nz/asciidoc/a2x.1.html) states that it can
>>> generate "PDF, EPUB, DVI, PS, LaTeX, XHTML (single page or chunked), man
>>> page, HTML Help or plain text formats using asciidoc(1) and other
>>> applications (see REQUISITES section). SOURCE_FILE can also be a DocBook
>>> file with an .xml extension."
>>>
>>
>> AFAICS, for all cases other than HTML output, the "other applications"
>> are basically the docbook toolchain.
>>
>
> I just started looking at , which seems to be the newer way to handle
> asciidoc. Aside from being a lot faster than a2x/asciidoc, it can produce
> docbook natively. However...
>
> What I expect would be a lot more effort is actually converting all the
>>> SGML to asciidoc. A quick google search doesn't turn up anything
>>> promising.
>>>
>>
>> Yeah, the cost of conversion means we're not likely to want to experiment
>> to see what's better :-(.
>>
>
> If the only concern is handling docbook format (which is what our SGML
> docs produce? Then https://github.com/asciidoctor/asciidoctor-fopub might
> be an option. It's intended for use with asciidoctor, but the README does
> state:
>
> "Using the asciidoctor-fopub project, you can convert any DocBook file
> into a nicely formatted PDF with nothing more than a Java runtime (JVM) and
> development kit (JDK). All the open source software required to perform the
> conversion is automatically fetched from the internet the first time you
> run it."
>
> So maybe it would allow removing some of more problematic parts of the
> toolchain?
>
> Also, if our SGML does produce docbook as an intermediate it might be
> possible to convert that to asciidoc via
> https://github.com/oreillymedia/docbook2asciidoc.


There's a misconception there...

Our SGML *is* DocBook.  Natively, no translation needed.

DocBook is a document type, and our documentation is already written using
that document type (DOCTYPE).  Easily seen thus:

postgres@cbbrowne ~/p/d/s/sgml> grep DOCTYPE
postgres.sgml
master!?
 asciidoc ---> DocBook
transition.

The trouble that we have is that what we have isn't a "DocBook file", but
rather a fairly large set of files representing a DocBook document.

I'm not sure what improvement we'd get out of using asciidoctor-fopub.

There would be some merit to some remapping to transform "creaky old
DocBook 4.2" (what we're using) to a newer version, perhaps biased towards
XML, and have our toolset merge the bits into a big XML (in DocBook 5,
presumably) file for processing using more modern DocBook tools.

I could probably build some DSSSL as helper (my HTML-to-DocBook DSSSL was
highly incomplete, and nonetheless surprisingly widely referenced for
years...), but we'd best be clear on what we think we're getting as
improvement.  Switching to a less expressive format is unlikely to be a
win, however creaky the current DocBook/DSSSL tools are.
-- 
When confronted by a difficult problem, solve it by reducing it to the
question, "How would the Lone Ranger handle this?"

Re: [HACKERS] INSERT ... ON CONFLICT documentation clean-up patch

On 2015-10-11 17:54:24 -0700, Peter Geoghegan wrote:
> +
> + 
> +  INSERT statements with ON
> +  CONFLICT clauses do not work sensibly with the partitioning
> +  schemes shown here, since trigger functions are not currently
> +  capable of examining the structure of the original
> +  INSERT.  In particular, trigger functions
> +  have no way to determine how the INSERT
> +  command's author might have intended redirected
> +  inserts to handle conflicts in child tables.  Even the
> +  involvement of an ON CONFLICT clause is not exposed
> +  to trigger functions.
> + 
> +
> +

"do not work sensibly" imo doesn't sound very good in docs. Maybe
something roughly along the lines of "are unlikely to work as expected
as the on conflict action is only taken in case of unique violation on
the target relation, not child relations". I'd remove the whole bit
about triggers not having access to ON CONFLICT.

> 
> 
>  
> diff --git a/doc/src/sgml/ref/insert.sgml b/doc/src/sgml/ref/insert.sgml
> index 8caf5fe..0794acb3 100644
> --- a/doc/src/sgml/ref/insert.sgml
> +++ b/doc/src/sgml/ref/insert.sgml
> @@ -99,7 +99,8 @@ INSERT INTO  class="PARAMETER">table_name [ AS 
> You must have INSERT privilege on a table in
> order to insert into it.  If ON CONFLICT DO UPDATE is
> -   present the UPDATE privilege is also required.
> +   present, UPDATE privilege on the table is also
> +   required.
>

Is this actually an improvement?

>
> @@ -161,10 +162,7 @@ INSERT INTO  class="PARAMETER">table_name [ AS   
>   
>A substitute name for the target table. When an alias is provided, it
> -  completely hides the actual name of the table.  This is particularly
> -  useful when using ON CONFLICT DO UPDATE into a table
> -  named excluded as that's also the name of the
> -  pseudo-relation containing the proposed row.
> +  completely hides the actual name of the table.
>   
>  
> 

Hm?

> @@ -395,19 +393,23 @@ INSERT INTO  class="PARAMETER">table_name [ AS  conflict_target describes which conflicts
> are handled by the ON CONFLICT clause.  Either a
> unique index inference clause or an explicitly
> -   named constraint can be used.  For ON CONFLICT DO
> -   NOTHING, it is optional to specify a
> -   conflict_target; when omitted, conflicts
> -   with all usable constraints (and unique indexes) are handled.  For
> -   ON CONFLICT DO UPDATE, a conflict target
> -   must be specified.
> +   named constraint can be used.  These constraints and/or unique
> +   indexes are arbiter indexes.

I'm not convinced it's an improvement to talk about arbiter indexes
here.

> +   For
> +   ON CONFLICT DO NOTHING, it is optional to
> +   specify a conflict_target; when omitted,
> +   conflicts with all usable constraints (and unique indexes) are
> +   handled.  For ON CONFLICT DO UPDATE, a
> +   conflict_target must be
> +   provided.

Yes, that's a bit clearer.

> Every time an insertion without ON CONFLICT
> would ordinarily raise an error due to violating one of the
> -   inferred (or explicitly named) constraints, a conflict (as in
> -   ON CONFLICT) occurs, and the alternative action,
> -   as specified by conflict_action is taken.
> -   This happens on a row-by-row basis.
> +   inferred constraints/indexes (or an explicitly named constraint), a
> +   conflict (as in ON CONFLICT) occurs, and the
> +   alternative action, as specified by
> +   conflict_action is taken.  This happens on a
> +   row-by-row basis.  Only NOT DEFERRABLE
> +   constraints are supported as arbiters.
>

I'm inclined ot only add the "Only NOT DEFERRABLE 
constraints are
supported as arbiters." bit - the rest doesn't really seem to be an
improvement?

>
> @@ -425,17 +427,28 @@ INSERT INTO  class="PARAMETER">table_name [ AS  specified columns/expressions and, if specified, whose predicate
> implies the 
> index_predicate are chosen as arbiter indexes.  Note
> -   that this means an index without a predicate will be used if a
> -   non-partial index matching every other criteria happens to be
> -   available.
> +   that this means a unique index without a predicate will be inferred
> +   (and used by ON CONFLICT as an arbiter) if such
> +   an index satisfying every other criteria is available.
>

Hm. I agree that the "happens to be available" sounds a bit too
casual. But I think the sentence was easier to understand for endusers
before?

> +   
> +
> + A unique index inference clause should be preferred over naming a
> + constraint directly using ON CONFLICT ON
> + CONSTRAINT 
> + constraint_name.  Unique index inference is
> + adaptable to nonsignificant changes in the available constraints.
> + Furthermore, it is possible for more than one constraint and/or
> + unique index to be inferred for the same statement.
> +
> +   

I'd formulate this a more neutral. How

Re: [HACKERS] Allow ssl_renegotiation_limit in PG 9.5

Andres Freund wrote:
> On 2015-10-14 13:04:30 -0400, Tom Lane wrote:
> > It doesn't seem to me that a connector such as npgsql has any business
> > whatsoever fooling with such a parameter, unconditionally or otherwise.
> 
> I think in npgsql simply doesn't support renegotiation (IIRC because
> it'd have been hard to implement in .net). Which makes it somewhat
> reasonable to set it to 0.

I think we could continue to have the parameter except that it throws an
error if you try to set it to something other than 0.

-- 
Á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 wrote:
> On 2015-10-14 14:19:40 -0300, Alvaro Herrera wrote:
> > I think we could continue to have the parameter except that it throws an
> > error if you try to set it to something other than 0.
> 
> That'll make it hard to ever remove it tho.

Well, we just have to wait until 9.4 is out of support (so by the time
we're releasing 9.9, or 9.8 if we don't get release cycles under
control).  As far as I recall we still have the alias entry for sort_mem
in map_old_guc_names, and what grief does it cause?

> Not sure if it's worth doing so for a dubious use of the variable.

What would you recommend then?  Forcing the user to specify the version
before the connection is established is not nice.

-- 
Á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] Foreign join pushdown vs EvalPlanQual

On Wed, Oct 14, 2015 at 3:10 AM, Kyotaro HORIGUCHI
 wrote:
> AFAICS, no updated version for remote tables are obtained.

You're right, but that's OK: the previously-obtained tuples fail to
meet the current version of the quals, so there's no problem (that I
can see).

> Even though the behavior I described above is correct, the join
> would fail, too. But it is because v.r is no longer equal to
> bigft2.r in the whole-row-var tuples. This seems seemingly the
> same behavior with that on local tables.

Yeah.

-- 
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

2015-10-14 Thread Tom Lane

Andres Freund  writes:
> On 2015-10-14 18:53:14 +0300, Shay Rojansky wrote:
>> However, the new situation where some versions of PG allow this parameter
>> while others bomb when seeing it. Specifically, Npgsql sends
>> ssl_renegotiation_limit=0 in the startup packet to completely disable
>> renegotiation. At this early stage it doesn't know yet whether the database
>> it's connecting to is PG 9.5 or earlier.

> I find it a rather debatable practice to send such a parameter
> unconditionally. Why are you sending it before the connection has even
> been established?

It doesn't seem to me that a connector such as npgsql has any business
whatsoever fooling with such a parameter, unconditionally or otherwise.

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] [COMMITTERS] pgsql: Cause TestLib.pm to define $windows_os in all branches.

2015-10-14 Thread Michael Paquier

On Tue, Oct 13, 2015 at 10:17 PM, Andrew Dunstan wrote:
> In general I think we can be a good deal more liberal about backpatching the
> testing regime than we are with production code, where we are always
> cautious, and the caution has paid big dividends in our reputation for
> stability.

Attached are patches for 9.4 and 9.5 syncing up everything with
master. I tested both of them on OSX, Linux and Windows (MSVC only
though using vcregress tapcheck).
-- 
Michael
diff --git a/doc/src/sgml/install-windows.sgml b/doc/src/sgml/install-windows.sgml
index 9b77648..d3d8f5f 100644
--- a/doc/src/sgml/install-windows.sgml
+++ b/doc/src/sgml/install-windows.sgml
@@ -438,6 +438,7 @@ $ENV{CONFIG}="Debug";
 vcregress contribcheck
 vcregress ecpgcheck
 vcregress isolationcheck
+vcregress tapcheck
 vcregress upgradecheck
 
 
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index e92d3c6..333cb17 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -334,14 +334,14 @@ ifeq ($(enable_tap_tests),yes)
 
 define prove_installcheck
 rm -rf $(CURDIR)/tmp_check/log
-cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl
+cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl
 endef
 
 define prove_check
 rm -rf $(CURDIR)/tmp_check/log
 $(MKDIR_P) tmp_check/log
 $(MAKE) -C $(top_builddir) DESTDIR='$(CURDIR)'/tmp_check/install install >'$(CURDIR)'/tmp_check/log/install.log 2>&1
-cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(CURDIR)/tmp_check/install$(bindir):$$PATH" $(call add_to_path,$(ld_library_path_var),$(CURDIR)/tmp_check/install$(libdir)) top_builddir='$(CURDIR)/$(top_builddir)' PGPORT='6$(DEF_PGPORT)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl
+cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(CURDIR)/tmp_check/install$(bindir):$$PATH" $(call add_to_path,$(ld_library_path_var),$(CURDIR)/tmp_check/install$(libdir)) top_builddir='$(CURDIR)/$(top_builddir)' PGPORT='6$(DEF_PGPORT)' PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl
 endef
 
 else
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 538ca0a..b40f89a 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -25,11 +25,7 @@ if (open BADCHARS, ">>$tempdir/pgdata/FOO\xe0\xe0\xe0BAR")
 	close BADCHARS;
 }
 
-open HBA, ">>$tempdir/pgdata/pg_hba.conf";
-print HBA "local replication all trust\n";
-print HBA "host replication all 127.0.0.1/32 trust\n";
-print HBA "host replication all ::1/128 trust\n";
-close HBA;
+configure_hba_for_replication "$tempdir/pgdata";
 system_or_bail 'pg_ctl', '-D', "$tempdir/pgdata", 'reload';
 
 command_fails(
@@ -57,54 +53,60 @@ command_ok([ 'pg_basebackup', '-D', "$tempdir/tarbackup", '-Ft' ],
 	'tar format');
 ok(-f "$tempdir/tarbackup/base.tar", 'backup tar was created');
 
-# Create a temporary directory in the system location and symlink it
-# to our physical temp location.  That way we can use shorter names
-# for the tablespace directories, which hopefully won't run afoul of
-# the 99 character length limit.
-my $shorter_tempdir = tempdir_short . "/tempdir";
-symlink "$tempdir", $shorter_tempdir;
-
-mkdir "$tempdir/tblspc1";
-psql 'postgres', "CREATE TABLESPACE tblspc1 LOCATION '$shorter_tempdir/tblspc1';";
-psql 'postgres', "CREATE TABLE test1 (a int) TABLESPACE tblspc1;";
-command_ok([ 'pg_basebackup', '-D', "$tempdir/tarbackup2", '-Ft' ],
-	'tar format with tablespaces');
-ok(-f "$tempdir/tarbackup2/base.tar", 'backup tar was created');
-my @tblspc_tars = glob "$tempdir/tarbackup2/[0-9]*.tar";
-is(scalar(@tblspc_tars), 1, 'one tablespace tar was created');
-
-command_fails(
-	[ 'pg_basebackup', '-D', "$tempdir/backup1", '-Fp' ],
-	'plain format with tablespaces fails without tablespace mapping');
-
-command_ok(
-	[   'pg_basebackup','-D',
-		"$tempdir/backup1", '-Fp',
-		"-T$shorter_tempdir/tblspc1=$tempdir/tbackup/tblspc1" ],
-	'plain format with tablespaces succeeds with tablespace mapping');
-ok(-d "$tempdir/tbackup/tblspc1", 'tablespace was relocated');
-opendir(my $dh, "$tempdir/pgdata/pg_tblspc") or die;
-ok( (   grep
-		{
-			-l "$tempdir/backup1/pg_tblspc/$_"
-			  and readlink "$tempdir/backup1/pg_tblspc/$_" eq
-			  "$tempdir/tbackup/tblspc1"
-		  } readdir($dh)),
-	"tablespace symlink was updated");
-closedir $dh;
-
-mkdir "$tempdir/tbl=spc2";
-psql 'postgres', "DROP TABLE test1;";
-psql 'postgres', "DROP TABLESPACE tblspc1;";
-psql 'postgres', "CREATE TABLESPACE tblspc2 LOCATION '$shorter_tempdir/tbl=spc2';";
-command_ok(
-	[   'pg_basebackup','-D',
-		"$tempdir/backup3", '-Fp',
-

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

Hello,

At Fri, 9 Oct 2015 18:40:32 +0900, Etsuro Fujita  
wrote in <56178b90.4030...@lab.ntt.co.jp>
> > What do you think the right behavior?

# 'is' was omitted..

> IIUC, I think that the foreign scan's slot should be set empty, that

Even for the case, EvalPlanQualFetchRowMarks retrieves tuples of
remote tables out of the whole-row resjunks and set them to
es_epqTuple[] so that EvalPlanQualNext can use it. The behavior
is not different from the 'FOR UPDATE;' (for all tables) cases.

I supposed that whole-row value for the joined tuple would be
treated in the same manner to the case of the tuples of base
foreign relations.

This is because preprocess_rowmarks makes rowMarks of the type
LCS_NONE for the relations other than the designated by "OF
colref" for "FOR UPDATE". Then it is converted into ROW_MARK_COPY
by select_rowmark_type, which causes the behavior above, as the
default behavior for foreign tables.

> the join should fail, and that the updated version of the tuple in v
> should be ignored in that scenario since that for the updated version
> of the tuple in v, the tuples obtained from those two foreign tables
> wouldn't satisfy the remote query.

AFAICS, no updated version for remote tables are obtained.

Even though the behavior I described above is correct, the join
would fail, too. But it is because v.r is no longer equal to
bigft2.r in the whole-row-var tuples. This seems seemingly the
same behavior with that on local tables.

# LCS_NONE for local tables is converted into ROW_MARK_COPY if no
# securityQuals are attached.

> But if populating the foreign
> scan's slot from that column, then the join would success and the
> updated version of the tuple in v would be returned wrongly, I think.

I might understand wrongly in some points..

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



-- 
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: index-only scans with partial indexes

Hello,

At Fri, 09 Oct 2015 16:32:31 +0200, Tomas Vondra  
wrote in <5617cfff.10...@2ndquadrant.com>
> Hello,
> 
> On 10/09/2015 02:59 AM, Kyotaro HORIGUCHI wrote:
> >>> The cause of this seeming mismatch would be the place to hold
> >>> indexrinfos. It is determined only by baserestrictinfo and
> >>> indpred. Any other components are not involved. So IndexClauseSet
> >>> is found not to be the best place after all, I suppose.
> >>>
> >>> Instead, I came to think that the better place is
> >>> IndexOptInfo. Partial indexes are examined in check_partial_index
> >>> and it seems to be the most proper place to check this so far.
> >>
> >> AFAIK there's only one IndexOptInfo instance per index, so I'm not
> >> sure how would that work with queries that use the index in multiple
> >> places?
> >
> > No matter if the index is used multiple places, indexrinfos is
> > determined only with baserestrictinfos of the owner relation and
> > itself's indpred, which are invariant through the following steps.
> 
> I'm probably missing something, but let's say we have a table like
> this:

You might be missing the fact that a table could represented as
multiple relation(RelOptInfo)s in PlannerInfo or PlannerGlobal.

> CREATE TABLE t (a INT, b INT, c INT);
> CREATE INDEX aidx ON t(c) WHERE a = 1;
> CREATE INDEX bidx ON t(c) WHERE b = 2;
> 
> and then a trivial query (where each part perfectly matches one of the
> indexes to allow IOS)
> 
> SELECT c FROM t WHERE a=1
> UNION ALL
> SELECT c FROM t WHERE b=2;
> 
> Now, let's say we move indexrinfos to IndexOptInfo - how will that
> look like for each index? There's only a single IndexOptInfo for each
> index, so it will have to work with union of all baserestrictinfos.

Needless to say about IndexOptInfo, the two t's in the two
component SELECTS are represented as two different subquery rels
having different baserestrictinfo. So it will correctly be
planned as the following with my previous patch.

Append  (cost=0.12..64.66 rows=20 width=4)
   ->  Index Only Scan using aidx on t  (cost=0.12..32.23 rows=10 width=4)
   ->  Index Only Scan using bidx on t t_1  (cost=0.12..32.23 rows=10 width=4)
(3 rows)

The table t is referred to twice by different (alias) names
(though the diferrence is made by EXPLAIN, it shows that they are
different rels in plantree).

> So we'll have these indexrinfos:
> 
> aidx: {b=2}
> bidx: {a=1}

Yes, but each of them belongs to different rels. So,

> which makes index only scans unusable.

The are usable.

> I think we effectively need a separate list of "not implied" clauses
> per index-baserel combination.
> Maybe IndexClauseSet is not the right
> place, but I don't see how IndexOptInfo could work.

Does this make sense?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



-- 
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

At Wed, 14 Oct 2015 11:08:37 +0530, Amit Kapila  wrote 
in 
> On Tue, Oct 13, 2015 at 3:54 PM, Rajeev rastogi 
> wrote:
> > If we add the event WL_POSTMASTER_DEATH also, client backend process
> > handling will become same as other backend process. So postmaster death can
> > be detected in the same way.
> >
> > But I am not sure if WL_POSTMASTER_DEATH event was not added intentionally
> > for some reason. Please confirm.
> > 
> > Also is it OK to add this even handling in generic path of Libpq?
> >
> > Please let me know if I am missing something?
> >
> >
> I feel this is worth investigation, example for what kind of cases libpq is
> used for non-blocking sockets, because for such cases above idea
> will not work.

Blocking mode of a port is changed using
socket_set_nonblocking(). I found two points that the function is
called with true.  pq_getbyte_if_available() and
socket_flush_if_writable(). They seems to be used only in
walsender *so far*.

> Here, I think the bigger point is that, Tom was not in favour of
> this proposal (making backends exit on postmaster death ) at that time,
> not sure whether he has changed his mind.

If I recall correctly, he concerned about killing the backends
running transactions which could be saved. I have a sympathy with
the opinion. But also think it reasonable to kill all backends
immediately so that new postmaster can run...

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



-- 
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] Foreign join pushdown vs EvalPlanQual

2015-10-14 Thread Etsuro Fujita

On 2015/10/14 12:07, Kouhei Kaigai wrote:
>> On 2015/10/07 15:39, Etsuro Fujita wrote:
>> I noticed that the approach using a column to populate the foreign
>> scan's slot directly wouldn't work well in some cases.  For example,
>> consider:
>>
>> SELECT * FROM verysmall v LEFT JOIN (bigft1 JOIN bigft2 ON bigft1.x =
>> bigft2.x) ON v.q = bigft1.q AND v.r = bigft2.r FOR UPDATE OF v;
>>
>> The best plan is presumably something like this as you said before:
>>
>> LockRows
>> -> Nested Loop
>>  -> Seq Scan on verysmall v
>>  -> Foreign Scan on bigft1 and bigft2
>>   Remote SQL: SELECT * FROM bigft1 JOIN bigft2 ON bigft1.x =
>> bigft2.x AND bigft1.q = $1 AND bigft2.r = $2
>>
>> Consider the EvalPlanQual testing to see if the updated version of a
>> tuple in v satisfies the query.  If we use the column in the testing, we
>> would get the wrong results in some cases.

> In this case, does ForeignScan have to be reset prior to ExecProcNode()?
> Once ExecReScanForeignScan() gets called by ExecNestLoop(), it marks EPQ
> slot is invalid. So, more or less, ForeignScan needs to kick the remote
> join again based on the new parameter come from the latest verysmall tuple.
> Please correct me, if I don't understand correctly.
> In case of unparametalized ForeignScan case, the cached join-tuple work
> well because it is independent from verysmall.
> 
> Once again, if FDW driver is responsible to construct join-tuple from
> the base relation's tuple cached in EPQ slot, this case don't need to
> kick remote query again, because all the materials to construct join-
> tuple are already held locally. Right?

Sorry, maybe I misunderstand your words, but we are talking here about
an approach using a whole-row var that would populate a join tuple that
is returned by an FDW and stored in the scan slot in the corresponding
ForeingScanState node in the parent state tree.

Best regards,
Etsuro Fujita



-- 
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] Foreign join pushdown vs EvalPlanQual

Hello,

At Wed, 14 Oct 2015 03:07:31 +, Kouhei Kaigai  wrote 
in <9a28c8860f777e439aa12e8aea7694f801157...@bpxm15gp.gisp.nec.co.jp>
> > I noticed that the approach using a column to populate the foreign
> > scan's slot directly wouldn't work well in some cases.  For example,
> > consider:
> > 
> > SELECT * FROM verysmall v LEFT JOIN (bigft1 JOIN bigft2 ON bigft1.x =
> > bigft2.x) ON v.q = bigft1.q AND v.r = bigft2.r FOR UPDATE OF v;
> > 
> > The best plan is presumably something like this as you said before:
> > 
> > LockRows
> > -> Nested Loop
> > -> Seq Scan on verysmall v
> > -> Foreign Scan on bigft1 and bigft2
> >  Remote SQL: SELECT * FROM bigft1 JOIN bigft2 ON bigft1.x =
> > bigft2.x AND bigft1.q = $1 AND bigft2.r = $2
> > 
> > Consider the EvalPlanQual testing to see if the updated version of a
> > tuple in v satisfies the query.  If we use the column in the testing, we
> > would get the wrong results in some cases.

I have a basic (or maybe silly) qustion. Is it true that the
join-inner (the foreignscan in the example) is re-executed with
the modified value of v.r?  I observed for a join case among only
local tables that previously fetched tuples for the inner are
simplly reused regardless of join types. Even when a refetch
happens (I haven't confirmed but it would occur in the case of no
security quals), the tuple is pointed by ctid so the re-join
between local and remote would fail. Is this wrong?

> In this case, does ForeignScan have to be reset prior to ExecProcNode()?
> Once ExecReScanForeignScan() gets called by ExecNestLoop(), it marks EPQ
> slot is invalid. So, more or less, ForeignScan needs to kick the remote
> join again based on the new parameter come from the latest verysmall tuple.
> Please correct me, if I don't understand correctly.

So, no rescan would happen for the cases, I think. ReScan seems
to be kicked only for the new(next) outer tuple that causes
change of parameter, but not kicked for EPQ. I might take you
wrongly..

> In case of unparametalized ForeignScan case, the cached join-tuple work
> well because it is independent from verysmall.


> Once again, if FDW driver is responsible to construct join-tuple from
> the base relation's tuple cached in EPQ slot, this case don't need to
> kick remote query again, because all the materials to construct join-
> tuple are already held locally. Right?

It is definitely right and should be doable. But I think the
point we are argueing here is what is the desirable behavior.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



-- 
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] Database schema diff

2015-10-14 Thread Shulgin, Oleksandr

On Tue, Oct 13, 2015 at 5:48 PM, Michal Novotny <
michal.novo...@trustport.com> wrote:

> Hi guys,
>
> I would like to ask you whether is there any tool to be able to compare
> database schemas ideally no matter what the column order is or to dump
> database table with ascending order of all database columns.
>
> For example, if I have table (called table) in schema A and in schema B
> (the time difference between is 1 week) and I would like to verify the
> column names/types matches but the order is different, i.e.:
>
> Schema A (2015-10-01) |  Schema B (2015-10-07)
>   |
> id int|  id int
> name varchar(64)  |  name varchar(64)
> text text |  description text
> description text  |  text text
>
> Is there any tool to compare and (even in case above) return that both
> tables match? Something like pgdiff or something?
>
> This should work for all schemas, tables, functions, triggers and all
> the schema components?
>

I've used pg_dump --split for this purpose a number of times (it requires
patching pg_dump[1]).

The idea is to produce the two database's schema dumps split into
individual files per database object, then run diff -r against the schema
folders.  This worked really well for my purposes.

This will however report difference in columns order, but I'm not really
sure why would you like to ignore that.

--
Alex

[1]
http://www.postgresql.org/message-id/AANLkTikLHA2x6U=q-t0j0ys78txhfmdtyxjfsrsrc...@mail.gmail.com

[HACKERS] Typo in replorigin_sesssion_origin (9.5+)

2015-10-14 Thread Craig Ringer

Hi all

Before 9.5 goes final, lets change replorigin_sesssion_origin and
replorigin_sesssion_origin_lsn to remove the extra 's'.

Yes, it's trivial.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From 5e2abf558e0b68064aa4f5012d023a76786c2778 Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Wed, 14 Oct 2015 16:47:20 +0800
Subject: [PATCH] Spelling fix: replorigin_sesssion_origin

---
 src/backend/access/transam/xact.c| 20 ++--
 src/backend/access/transam/xloginsert.c  |  6 +++---
 src/backend/replication/logical/origin.c | 26 +-
 src/include/replication/origin.h |  6 +++---
 4 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index b53d95f..5c2966d 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1210,12 +1210,12 @@ RecordTransactionCommit(void)
 		 * Record plain commit ts if not replaying remote actions, or if no
 		 * timestamp is configured.
 		 */
-		if (replorigin_sesssion_origin == InvalidRepOriginId ||
-			replorigin_sesssion_origin == DoNotReplicateId ||
-			replorigin_sesssion_origin_timestamp == 0)
-			replorigin_sesssion_origin_timestamp = xactStopTimestamp;
+		if (replorigin_session_origin == InvalidRepOriginId ||
+			replorigin_session_origin == DoNotReplicateId ||
+			replorigin_session_origin_timestamp == 0)
+			replorigin_session_origin_timestamp = xactStopTimestamp;
 		else
-			replorigin_session_advance(replorigin_sesssion_origin_lsn,
+			replorigin_session_advance(replorigin_session_origin_lsn,
 	   XactLastRecEnd);
 
 		/*
@@ -1224,8 +1224,8 @@ RecordTransactionCommit(void)
 		 * action during replay.
 		 */
 		TransactionTreeSetCommitTsData(xid, nchildren, children,
-	   replorigin_sesssion_origin_timestamp,
-	   replorigin_sesssion_origin, false);
+	   replorigin_session_origin_timestamp,
+	   replorigin_session_origin, false);
 	}
 
 	/*
@@ -5133,12 +5133,12 @@ XactLogCommitRecord(TimestampTz commit_time,
 	}
 
 	/* dump transaction origin information */
-	if (replorigin_sesssion_origin != InvalidRepOriginId)
+	if (replorigin_session_origin != InvalidRepOriginId)
 	{
 		xl_xinfo.xinfo |= XACT_XINFO_HAS_ORIGIN;
 
-		xl_origin.origin_lsn = replorigin_sesssion_origin_lsn;
-		xl_origin.origin_timestamp = replorigin_sesssion_origin_timestamp;
+		xl_origin.origin_lsn = replorigin_session_origin_lsn;
+		xl_origin.origin_timestamp = replorigin_session_origin_timestamp;
 	}
 
 	if (xl_xinfo.xinfo != 0)
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index abd8420..925255f 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -701,11 +701,11 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
 	}
 
 	/* followed by the record's origin, if any */
-	if (include_origin && replorigin_sesssion_origin != InvalidRepOriginId)
+	if (include_origin && replorigin_session_origin != InvalidRepOriginId)
 	{
 		*(scratch++) = XLR_BLOCK_ID_ORIGIN;
-		memcpy(scratch, _sesssion_origin, sizeof(replorigin_sesssion_origin));
-		scratch += sizeof(replorigin_sesssion_origin);
+		memcpy(scratch, _session_origin, sizeof(replorigin_session_origin));
+		scratch += sizeof(replorigin_session_origin);
 	}
 
 	/* followed by main data, if any */
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index c219590..a6a1e4b 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -148,9 +148,9 @@ typedef struct ReplicationStateCtl
 } ReplicationStateCtl;
 
 /* external variables */
-RepOriginId replorigin_sesssion_origin = InvalidRepOriginId;	/* assumed identity */
-XLogRecPtr	replorigin_sesssion_origin_lsn = InvalidXLogRecPtr;
-TimestampTz replorigin_sesssion_origin_timestamp = 0;
+RepOriginId replorigin_session_origin = InvalidRepOriginId;	/* assumed identity */
+XLogRecPtr	replorigin_session_origin_lsn = InvalidXLogRecPtr;
+TimestampTz replorigin_session_origin_timestamp = 0;
 
 /*
  * Base address into a shared memory array of replication states of size
@@ -803,7 +803,7 @@ replorigin_redo(XLogReaderState *record)
  * Tell the replication origin progress machinery that a commit from 'node'
  * that originated at the LSN remote_commit on the remote node was replayed
  * successfully and that we don't need to do so again. In combination with
- * setting up replorigin_sesssion_origin_lsn and replorigin_sesssion_origin
+ * setting up replorigin_session_origin_lsn and replorigin_session_origin
  * that ensures we won't loose knowledge about that after a crash if the
  * transaction had a persistent effect (think of asynchronous commits).
  *
@@ -1236,7 +1236,7 @@ pg_replication_origin_session_setup(PG_FUNCTION_ARGS)
 	origin =

Re: [HACKERS] Database schema diff

2015-10-14 Thread Torello Querci

Few years ago I developed a tool called fsgateway (
https://github.com/mk8/fsgateway) that show metadata (table, index,
sequences, view) as normal files using fuse.
In this way to yout can get differences between running db instance using
diff, meld or what do you prefear.

Unfortunally at the moment not all you need is supported, yet.

Best regards

P.S. I think that this is the wrong list for questione like this one.

On Wed, Oct 14, 2015 at 10:26 AM, Shulgin, Oleksandr <
oleksandr.shul...@zalando.de> wrote:

> On Tue, Oct 13, 2015 at 5:48 PM, Michal Novotny <
> michal.novo...@trustport.com> wrote:
>
>> Hi guys,
>>
>> I would like to ask you whether is there any tool to be able to compare
>> database schemas ideally no matter what the column order is or to dump
>> database table with ascending order of all database columns.
>>
>> For example, if I have table (called table) in schema A and in schema B
>> (the time difference between is 1 week) and I would like to verify the
>> column names/types matches but the order is different, i.e.:
>>
>> Schema A (2015-10-01) |  Schema B (2015-10-07)
>>   |
>> id int|  id int
>> name varchar(64)  |  name varchar(64)
>> text text |  description text
>> description text  |  text text
>>
>> Is there any tool to compare and (even in case above) return that both
>> tables match? Something like pgdiff or something?
>>
>> This should work for all schemas, tables, functions, triggers and all
>> the schema components?
>>
>
> I've used pg_dump --split for this purpose a number of times (it requires
> patching pg_dump[1]).
>
> The idea is to produce the two database's schema dumps split into
> individual files per database object, then run diff -r against the schema
> folders.  This worked really well for my purposes.
>
> This will however report difference in columns order, but I'm not really
> sure why would you like to ignore that.
>
> --
> Alex
>
> [1]
> http://www.postgresql.org/message-id/AANLkTikLHA2x6U=q-t0j0ys78txhfmdtyxjfsrsrc...@mail.gmail.com
>
>

Re: [HACKERS] Use pg_rewind when target timeline was switched

2015-10-14 Thread Alexander Korotkov

On Wed, Sep 30, 2015 at 9:46 AM, Michael Paquier 
wrote:

> On Sat, Sep 19, 2015 at 8:27 AM, Michael Paquier wrote:
> > On Fri, Sep 18, 2015 at 6:25 PM, Michael Paquier wrote:
> >> The refactoring of getTimelineHistory as you propose looks like a good
> >> idea to me, I tried to remove by myself the difference between source
> >> and target in copy_fetch.c and friends but this gets uglier,
> >> particularly because of datadir_source in copy_file_range. Not worth
> >> it.
> >
> > Forgot that:
> > if (ControlFile_target.state != DB_SHUTDOWNED)
> > pg_fatal("target server must be shut down cleanly\n");
> > We may want to allow a target node shutdowned in recovery as well here.
>
> So, attached is a more polished version of this patch, cleaned up of
> its typos with as well other things. I have noticed for example that
> it would be more useful to add the debug information of a timeline
> file fetched from the source or a target server directly in
> getTimelineHistory. I have as well updated a couple of comments in the
> code regarding the fact that we do not necessarily use a master as a
> target node, and mentioned in findCommonAncestorTimeline that we check
> as well the start position of a timeline to cover the case where both
> target and source node forked at the same timeline number but with a
> different WAL fork position.
> I am marking this patch as ready for committer. It would be cool in
> the future to use the recovery test suite to have more advanced
> scenarios tested, but it seems a shame to block this patch because of
> that.
>

Thanks a lot. Now patch looks much better.
I found that it doesn't applies cleanly on the current master. Rebased
version is attached.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


pg-rewind-target-switch-6.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] Foreign join pushdown vs EvalPlanQual

2015-10-14 Thread Etsuro Fujita


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

On Thu, Oct 8, 2015 at 11:00 PM, Etsuro Fujita
 wrote:

The best plan is presumably something like this as you said before:

LockRows
-> Nested Loop
 -> Seq Scan on verysmall v
 -> Foreign Scan on bigft1 and bigft2
  Remote SQL: SELECT * FROM bigft1 JOIN bigft2 ON bigft1.x =
bigft2.x AND bigft1.q = $1 AND bigft2.r = $2

Consider the EvalPlanQual testing to see if the updated version of a
tuple in v satisfies the query.  If we use the column in the testing, we
would get the wrong results in some cases.



More precisely, we would get the wrong result when the value of v.q or v.r
in the updated version has changed.



Interesting test case.  It's worth considering why this works if you
were to replace the Foreign Scan with an Index Scan; suppose the query
is SELECT * FROM verysmall v LEFT JOIN realbiglocaltable t ON v.x =
t.x FOR UPDATE OF v, so that you get:

LockRows
-> Nested Loop
   -> Seq Scan on verysmall v
   -> Foreign Scan on realbiglocaltable t
 Index Cond: v.x = t.x

In your example, the remote SQL pushes down certain quals to the
remote server, and so if we just return the same tuple they might no
longer be satisfied.  In this example, the index qual is essentially a
filter condition that has been "pushed down" into the index AM.  The
EvalPlanQual machinery prevents this from generating wrong answers by
rechecking the index cond - see IndexRecheck.  Even though it's
normally the AM's job to enforce the index cond, and the executor does
not need to recheck, in the EvalPlanQual case it does need to recheck.

I think the foreign data wrapper case should be handled the same way.
Any condition that we initially pushed down to the foreign server
needs to be locally rechecked if we're inside EPQ.


Agreed.

As KaiGai-san also pointed out before, I think we should address this in 
each of the following cases:


1) remote qual (scanrelid>0)
2) remote join (scanrelid==0)

As for #1, I noticed that there is a bug in handling the same kind of 
FDW queries, which will be shown below.  As you said, I think this 
should be addressed by rechecking the remote quals *locally*.  (I 
thought another fix for this kind of bug before, though.)  IIUC, I think 
this should be fixed separately from #2, as this is a bug not only in 
9.5, but in back branches.  Please find attached a patch.


Create an environment:

mydatabase=# create table t1 (a int primary key, b text);
mydatabase=# insert into t1 select a, 'notsolongtext' from 
generate_series(1, 100) a;


postgres=# create server myserver foreign data wrapper postgres_fdw 
options (dbname 'mydatabase');

postgres=# create user mapping for current_user server myserver;
postgres=# create foreign table ft1 (a int, b text) server myserver 
options (table_name 't1');

postgres=# alter foreign table ft1 options (add use_remote_estimate 'true');
postgres=# create table inttab (a int);
postgres=# insert into inttab select a from generate_series(1, 10) a;
postgres=# analyze ft1;
postgres=# analyze inttab;

Run concurrent transactions that produce incorrect result:

[Terminal1]
postgres=# begin;
BEGIN
postgres=# update inttab set a = a + 1 where a = 1;
UPDATE 1

[Terminal2]
postgres=# explain verbose select * from inttab, ft1 where inttab.a = 
ft1.a limit 1 for update;

   QUERY PLAN
-
 Limit  (cost=100.43..198.99 rows=1 width=70)
   Output: inttab.a, ft1.a, ft1.b, inttab.ctid, ft1.*
   ->  LockRows  (cost=100.43..1086.00 rows=10 width=70)
 Output: inttab.a, ft1.a, ft1.b, inttab.ctid, ft1.*
 ->  Nested Loop  (cost=100.43..1085.90 rows=10 width=70)
   Output: inttab.a, ft1.a, ft1.b, inttab.ctid, ft1.*
   ->  Seq Scan on public.inttab  (cost=0.00..1.10 rows=10 
width=10)

 Output: inttab.a, inttab.ctid
   ->  Foreign Scan on public.ft1  (cost=100.43..108.47 
rows=1 width=18)

 Output: ft1.a, ft1.b, ft1.*
 Remote SQL: SELECT a, b FROM public.t1 WHERE 
(($1::integer = a)) FOR UPDATE

(11 rows)

postgres=# select * from inttab, ft1 where inttab.a = ft1.a limit 1 for 
update;


[Terminal1]
postgres=# commit;
COMMIT

[Terminal2]
(After the commit in Terminal1, the following result will be shown in 
Terminal2.  Note that the values of inttab.a and ft1.a wouldn't satisfy 
the remote qual!)

 a | a |   b
---+---+---
 2 | 1 | notsolongtext
(1 row)

As for #2, I didn't come up with any solution to locally rechecking 
pushed-down join conditions against a joined tuple populated from a 
column that we discussed.  Instead, I'd like to revise a 
local-join-execution-plan-based approach that we discussed before, by 
addressing your comments such as [1].  Would it be the right way to go?


Best regards,
Etsuro Fujita

[1]

Re: [HACKERS] Support for N synchronous standby servers - take 2

2015-10-14 Thread Beena Emerson

On Wed, Oct 14, 2015 at 10:38 AM, Michael Paquier  wrote:

> On Wed, Oct 14, 2015 at 3:02 AM, Masahiko Sawada wrote:
>
> > It would be good even if there are some restriction such as the
> > nesting level, the group setting.
> > The another new approach that I came up with is,
> > * Add new parameter synchronous_replication_method (say s_r_method)
> > which can have two names: 'priority', 'quorum'
> > * If s_r_method = 'priority', the value of s_s_names (e.g. 'n1,n2,n3')
> > is handled using priority. It's same as '[n1,n2,n3]' in dedicated
> > language.
> > * If s_r_method = 'quorum', the value of s_s_names is handled using
> > quorum commit, It's same as '(n1,n2,n3)' in dedicated language.
> > * Setting of synchronous_standby_names is same as today. That is, the
> > storing the nesting value is not supported.
> > * If we want to support more complex syntax like what we are
> > discussing, we can add the new value to s_r_method, for example
> > 'complex', 'json'.
>
> If we go that path, I think that we still would need an extra
> parameter to control the number of nodes that need to be taken from
> the set defined in s_s_names whichever of quorum or priority is used.
> Let's not forget that in the current configuration the first node
> listed in s_s_names and *connected* to the master will be used to
> acknowledge the commit.
>

Would it be better to just use a simple language instead of 3 different
parameters?

s_s_names = 2[X,Y,Z]  # 2 priority
s_s_names = 1(A,B,C) # 1 quorum
s_s_names = R,S,T # default behavior: 1 priorty?

Re: [HACKERS] pgbench - allow backslash-continuations in custom scripts

2015-10-14 Thread Fabien COELHO



Hello,

Here is a review, sorry for the delay...


This is done as the additional fourth patch, not merged into
previous ones, to show what's changed in the manner of command
storing.
[...]

 - SQL multi-statement.

   SELECT 1; SELECT 2;


I think this is really "SELECT 1\; SELECT 2;"

I join a test script I used.


The purpose of this 4 parts patch is to reuse psql scanner from pgbench
so that commands are cleanly separated by ";", including managing dollar
quoting, having \ continuations in backslash-commands, having 
multi-statement commands...


This review is about 4 part v4 of the patch. The patches apply and compile 
cleanly.


I think that the features are worthwhile. I would have prefer more limited
changes to get them, but my earlier attempt was rejected, and the scanner
sharing with psql results in reasonably limited changes, so I would go for
it.


* 0001 patch about psql scanner reworking

The relevant features lexer which can be reused by pgbench are isolated
and adapted thanks to ifdefs, guards, and putting some stuff in the
current state.

I'm not sure of the "OUTSIDE_PSQL" macro name. ISTM that "PGBENCH_SCANNER"
would be better, as it makes it very clear that it is being used for pgbench,
and if someone wants to use it for something else they should define and handle
their own case explicitely.

Use "void *" instead of "int *" for VariableSpace?

Rule ":{variable_char}+": the ECHO works more or less as a side effect,
and most of the code is really ignored by pgbench. Instead of the different
changes which rely on NULL values, what about a simple ifdef/else/endif
to replace the whole stuff by ECHO for pgbench, without changing the current
code?

Also, on the same part of the code, I'm not sure about having two assignments
on the "const char * value" variable, because of "const".

The "db" parameter is not used by psql_scan_setup, so the state's db field
is never initialized, so probably "psql" does not work properly because
it seems used in several places.

I'm not sure what would happen if there are reconnections from psql (is 
that possible? Without reseting the scanner state?), as there are two 
connections, one in pset and one in the scanner state?


Variable lookup guards: why is a database connection necessary for doing
":variables" lookups? It seemed not to be required in the initial version,
and is not really needed.

Avoid changing style without clear motivation, for instance the 
PQerrorMessage/psql_error on escape_value==NULL?



*** 0002 patch to use the scanner in pgbench

There is no documentation nor examples about the new features.
I think that the internal builtin commands and the documentation should
be used to show the new features where appropriate, and insist on that
";" are now required at the end of sql commands.

If the file starts with a -- comment followed by a backslash-command, eg:
  -- there is only one foo currently available
  \set foo 1
an error is reported: the comment should probably just be ignored.

I'm not sure that the special "next" command parsing management is useful.
I do not see a significant added value to managing especially a list of
commands for commands which happened to be on the same line but separated
by a simple ";". Could not the code be simplified by just restarting
the scanner where it left, instead of looping in "process_commands"?

It seems that part of the changes are just reindentations, especially
all the parsing code for backslash commands. This should be avoided if
possible.

Some spurious spaces after "next_command:".


*** 0003 patch for ms build

I don't do windows:-)

The perl script changes look pretty reasonable, although the copied
comments refer to pg_xlogdump, I guess it should rather refer to pgbench.


*** 0004 command list patch

This patch changes the command array to use a linked list.

As the command number is needed in several places and has to be replaced by a
function call which scans the list, I do not think it is a good idea, and
I recommand not to consider this part for inclusion.

--
Fabien.

test.sql
Description: application/sql

-- 
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] Foreign join pushdown vs EvalPlanQual

Ah.. 

I understood that what you mentioned is the lack of local recheck
of foreigh tuples. Sorry for the noise.


At Wed, 14 Oct 2015 17:31:16 +0900, Etsuro Fujita  
wrote in <561e12d4.7040...@lab.ntt.co.jp>
On 2015/10/10 10:17, Robert Haas wrote:
> > On Thu, Oct 8, 2015 at 11:00 PM, Etsuro Fujita
> >  wrote:
> >>> The best plan is presumably something like this as you said before:
> >>>
> >>> LockRows
> >>> -> Nested Loop
> >>>  -> Seq Scan on verysmall v
> >>>  -> Foreign Scan on bigft1 and bigft2
> >>>   Remote SQL: SELECT * FROM bigft1 JOIN bigft2 ON bigft1.x =
> >>> bigft2.x AND bigft1.q = $1 AND bigft2.r = $2
> >>>
> >>> Consider the EvalPlanQual testing to see if the updated version of a
> >>> tuple in v satisfies the query.  If we use the column in the testing,
> >>> we
> >>> would get the wrong results in some cases.
> 
> >> More precisely, we would get the wrong result when the value of v.q or
> >> v.r
> >> in the updated version has changed.
> 
> > Interesting test case.  It's worth considering why this works if you
> > were to replace the Foreign Scan with an Index Scan; suppose the query
> > is SELECT * FROM verysmall v LEFT JOIN realbiglocaltable t ON v.x =
> > t.x FOR UPDATE OF v, so that you get:
> >
> > LockRows
> > -> Nested Loop
> >-> Seq Scan on verysmall v
> >-> Foreign Scan on realbiglocaltable t
> >  Index Cond: v.x = t.x
> >
> > In your example, the remote SQL pushes down certain quals to the
> > remote server, and so if we just return the same tuple they might no
> > longer be satisfied.  In this example, the index qual is essentially a
> > filter condition that has been "pushed down" into the index AM.  The
> > EvalPlanQual machinery prevents this from generating wrong answers by
> > rechecking the index cond - see IndexRecheck.  Even though it's
> > normally the AM's job to enforce the index cond, and the executor does
> > not need to recheck, in the EvalPlanQual case it does need to recheck.
> >
> > I think the foreign data wrapper case should be handled the same way.
> > Any condition that we initially pushed down to the foreign server
> > needs to be locally rechecked if we're inside EPQ.
> 
> Agreed.
> 
> As KaiGai-san also pointed out before, I think we should address this
> in each of the following cases:
> 
> 1) remote qual (scanrelid>0)
> 2) remote join (scanrelid==0)
> 
> As for #1, I noticed that there is a bug in handling the same kind of
> FDW queries, which will be shown below.  As you said, I think this
> should be addressed by rechecking the remote quals *locally*.  (I
> thought another fix for this kind of bug before, though.)  IIUC, I
> think this should be fixed separately from #2, as this is a bug not
> only in 9.5, but in back branches.  Please find attached a patch.
> 
> Create an environment:
> 
> mydatabase=# create table t1 (a int primary key, b text);
> mydatabase=# insert into t1 select a, 'notsolongtext' from
> generate_series(1, 100) a;
> 
> postgres=# create server myserver foreign data wrapper postgres_fdw
> options (dbname 'mydatabase');
> postgres=# create user mapping for current_user server myserver;
> postgres=# create foreign table ft1 (a int, b text) server myserver
> options (table_name 't1');
> postgres=# alter foreign table ft1 options (add use_remote_estimate
> 'true');
> postgres=# create table inttab (a int);
> postgres=# insert into inttab select a from generate_series(1, 10) a;
> postgres=# analyze ft1;
> postgres=# analyze inttab;
> 
> Run concurrent transactions that produce incorrect result:
> 
> [Terminal1]
> postgres=# begin;
> BEGIN
> postgres=# update inttab set a = a + 1 where a = 1;
> UPDATE 1
> 
> [Terminal2]
> postgres=# explain verbose select * from inttab, ft1 where inttab.a =
> ft1.a limit 1 for update;
>QUERY PLAN
> -
>  Limit  (cost=100.43..198.99 rows=1 width=70)
>Output: inttab.a, ft1.a, ft1.b, inttab.ctid, ft1.*
>->  LockRows  (cost=100.43..1086.00 rows=10 width=70)
>  Output: inttab.a, ft1.a, ft1.b, inttab.ctid, ft1.*
>  ->  Nested Loop  (cost=100.43..1085.90 rows=10 width=70)
>Output: inttab.a, ft1.a, ft1.b, inttab.ctid, ft1.*
>->  Seq Scan on public.inttab (cost=0.00..1.10 rows=10
>->  width=10)
>  Output: inttab.a, inttab.ctid
>->  Foreign Scan on public.ft1 (cost=100.43..108.47 rows=1
>->  width=18)
>  Output: ft1.a, ft1.b, ft1.*
>  Remote SQL: SELECT a, b FROM public.t1 WHERE
>  (($1::integer = a)) FOR UPDATE
> (11 rows)
> 
> postgres=# select * from inttab, ft1 where inttab.a = ft1.a limit 1
> for update;
> 
> [Terminal1]
> postgres=# commit;
> COMMIT
> 
> [Terminal2]
> (After the commit in Terminal1,

Re: [HACKERS] Use pg_rewind when target timeline was switched

2015-10-14 Thread Alexander Korotkov

On Sat, Sep 19, 2015 at 2:25 AM, Michael Paquier 
wrote:

> OK, I see your point and you are right. This additional check allows
> pg_rewind to switch one timeline back and make the scan of blocks
> begin at the real origin of both timelines. I had in mind the case
> where you tried to actually rewind node 2 to node 3 actually which
> would not be possible with your patch, and by thinking more about that
> I think that it could be possible to remove the check I am listing
> below and rely on the checks in the history files, basically what is
> in findCommonAncestorTimeline:
> if (ControlFile_target.checkPointCopy.ThisTimeLineID ==
> ControlFile_source.checkPointCopy.ThisTimeLineID)
> pg_fatal("source and target cluster are on the same
> timeline\n");
> Alexander, what do you think about that? I think that we should be
> able to rewind with for example node 2 as target and node 3 as source,
> and vice-versa as per the example you gave even if both nodes are on
> the same timeline, just that they forked at a different point. Could
> you test this particular case? Using my script, simply be sure to
> switch archive_mode to on/off depending on the node, aka only 3 and 4
> do archiving.
>

I think relying on different fork point is not safe enough. Imagine more
complex case.

  1
 / \
2   3
|   |
4   5

At first, nodes 2 and 3 are promoted at the same point and they both get
timeline 2.
Then nodes 4 and 5 are promoted at different points and they both get
timeline 3.
Then we can try to rewind node 4 with node 5 as the source or vice versa.
In this case we can't find collision of timeline 2.

The same collision could happen even when source and target are on the
different timeline number. However, having the on the same timeline numbers
is suspicious enough to disallow it until we have additional checks.

I could propose following plan:

   1. Commit this patch without allowing rewind when target and source are
   on the same timelines.
   2. Make additional checks for distinguish different timelines with the
   same numbers.
   3. Allow rewind when target and source are on the same timelines.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] pam auth - add rhost item

On Tue, Oct 13, 2015 at 4:12 PM, kolo hhmow  wrote:
> Yes, sorry. I was in hurry when I posted this message.
> I dont understand whay in CheckPAMAuth function only PAM_USER item is adding
> to pam information before authenticate?
> Wheter it would be a problem to set additional pam information like
> PAM_RHOST which is very useful because we can use this item to restrict
> access to this ip address.
> I hope I'm more specific now and you will understand me.
> Sorry, but I'm not native english speaker.
> Patch in attachment, and link below to web-view on github:
> https://github.com/grzsmp/postgres/commit/5e2b102ec6de27e786d627623dcb187e997609e4

I don't personally know much about PAM, but if you want to restrict
access by IP, you could do that in pg_hba.conf.

-- 
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] Performance improvement for joins where outer side is unique

On Wed, Oct 14, 2015 at 1:03 AM, Pavel Stehule  wrote:
> it is great

+1.

-- 
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] pam auth - add rhost item

2015-10-14 Thread kolo hhmow

Yes, but this is very ugly solution, becasue you have to restart postgresql
daemon each time you have added a new user.
This solution which I propose is give an abbility to dinamicaly manage user
accounts without need to restart each time a user account entry has change.
When you have lot of actively users using postgresql service, you cannot
restart the server each time somebody add, or remove some user account
entry from the system.
This is whay we uses pam modules with pam-pgsql and with this patch.

On Wed, Oct 14, 2015 at 9:52 PM, Robert Haas  wrote:

> On Tue, Oct 13, 2015 at 4:12 PM, kolo hhmow  wrote:
> > Yes, sorry. I was in hurry when I posted this message.
> > I dont understand whay in CheckPAMAuth function only PAM_USER item is
> adding
> > to pam information before authenticate?
> > Wheter it would be a problem to set additional pam information like
> > PAM_RHOST which is very useful because we can use this item to restrict
> > access to this ip address.
> > I hope I'm more specific now and you will understand me.
> > Sorry, but I'm not native english speaker.
> > Patch in attachment, and link below to web-view on github:
> >
> https://github.com/grzsmp/postgres/commit/5e2b102ec6de27e786d627623dcb187e997609e4
>
> I don't personally know much about PAM, but if you want to restrict
> access by IP, you could do that in pg_hba.conf.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

On Wed, Oct 14, 2015 at 4:31 AM, Etsuro Fujita
 wrote:
> Agreed.
>
> As KaiGai-san also pointed out before, I think we should address this in
> each of the following cases:
>
> 1) remote qual (scanrelid>0)
> 2) remote join (scanrelid==0)
>
> As for #1, I noticed that there is a bug in handling the same kind of FDW
> queries, which will be shown below.  As you said, I think this should be
> addressed by rechecking the remote quals *locally*.  (I thought another fix
> for this kind of bug before, though.)  IIUC, I think this should be fixed
> separately from #2, as this is a bug not only in 9.5, but in back branches.
> Please find attached a patch.

+1 for doing something like this.  However, I don't think we can
commit this to released branches, despite the fact that it's a bug
fix, because breaking third-party FDWs in a minor release seems
unfriendly.  We might be able to slip it into 9.5, though, if we act
quickly.

A few review comments:

- nodeForeignscan.c now needs to #include "utils/memutils.h"
- I think it'd be safer for finalize_plan() not to try to shortcut
processing fdw_scan_quals.
- You forgot to update _readForeignScan.
- The documentation needs updating.
- I think we should use the name fdw_recheck_quals.

Here's an updated patch with those changes and some improvements to
the comments.  Absent objections, I will commit it and back-patch to
9.5 only.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index 499f24f..5ce8f90 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -563,7 +563,8 @@ fileGetForeignPlan(PlannerInfo *root,
 			scan_relid,
 			NIL,	/* no expressions to evaluate */
 			best_path->fdw_private,
-			NIL /* no custom tlist */ );
+			NIL,	/* no custom tlist */
+			NIL /* no remote quals */ );
 }
 
 /*
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index e4d799c..1902f1f 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -748,6 +748,7 @@ postgresGetForeignPlan(PlannerInfo *root,
 	Index		scan_relid = baserel->relid;
 	List	   *fdw_private;
 	List	   *remote_conds = NIL;
+	List	   *remote_exprs = NIL;
 	List	   *local_exprs = NIL;
 	List	   *params_list = NIL;
 	List	   *retrieved_attrs;
@@ -769,8 +770,8 @@ postgresGetForeignPlan(PlannerInfo *root,
 	 *
 	 * This code must match "extract_actual_clauses(scan_clauses, false)"
 	 * except for the additional decision about remote versus local execution.
-	 * Note however that we only strip the RestrictInfo nodes from the
-	 * local_exprs list, since appendWhereClause expects a list of
+	 * Note however that we don't strip the RestrictInfo nodes from the
+	 * remote_conds list, since appendWhereClause expects a list of
 	 * RestrictInfos.
 	 */
 	foreach(lc, scan_clauses)
@@ -784,11 +785,17 @@ postgresGetForeignPlan(PlannerInfo *root,
 			continue;
 
 		if (list_member_ptr(fpinfo->remote_conds, rinfo))
+		{
 			remote_conds = lappend(remote_conds, rinfo);
+			remote_exprs = lappend(remote_exprs, rinfo->clause);
+		}
 		else if (list_member_ptr(fpinfo->local_conds, rinfo))
 			local_exprs = lappend(local_exprs, rinfo->clause);
 		else if (is_foreign_expr(root, baserel, rinfo->clause))
+		{
 			remote_conds = lappend(remote_conds, rinfo);
+			remote_exprs = lappend(remote_exprs, rinfo->clause);
+		}
 		else
 			local_exprs = lappend(local_exprs, rinfo->clause);
 	}
@@ -874,7 +881,8 @@ postgresGetForeignPlan(PlannerInfo *root,
 			scan_relid,
 			params_list,
 			fdw_private,
-			NIL /* no custom tlist */ );
+			NIL,	/* no custom tlist */
+			remote_exprs);
 }
 
 /*
diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
index 4c410c7..1533a6b 100644
--- a/doc/src/sgml/fdwhandler.sgml
+++ b/doc/src/sgml/fdwhandler.sgml
@@ -1136,6 +1136,15 @@ GetForeignServerByName(const char *name, bool missing_ok);
 
 
 
+ Any clauses removed from the plan node's qual list must instead be added
+ to fdw_recheck_quals in order to ensure correct behavior
+ at the READ COMMITTED isolation level.  When a concurrent
+ update occurs for some other table involved in the query, the executor
+ may need to verify that all of the original quals are still satisfied for
+ the tuple, possibly against a different set of parameter values.
+
+
+
  Another ForeignScan field that can be filled by FDWs
  is fdw_scan_tlist, which describes the tuples returned by
  the FDW for this plan node.  For simple foreign table scans this can be
diff --git a/src/backend/executor/nodeForeignscan.c b/src/backend/executor/nodeForeignscan.c
index bb28a73..6165e4a 100644
--- a/src/backend/executor/nodeForeignscan.c
+++ b/src/backend/executor/nodeForeignscan.c
@@ -25,6 +25,7 @@
 #include "executor/executor.h"
 #include

Re: [HACKERS] [PROPOSAL] DIAGNOSTICS = SKIPPED_ROW_COUNT

On Tue, Oct 13, 2015 at 10:37 PM, dinesh kumar  wrote:
> In an existing wait policies like WAIT(default) and NO WAIT,
> one can be sure to determine(Using ROW_COUNT daignostics counter),
> how many required tuples he processed in a transaction.
> But this is not case when it comes to SKIP LOCKED.

Sure it is.  You didn't process the ones that you skipped.  This is no
different than if you say WHERE a = 5.  Depending on plan choice and
table contents, you may have "skipped" a large number of rows where a
!= 5, or you may have skipped none at all.

> In my view, SKIP LOCKED is a nice feature, which gives only the available OR
> unlocked tuples.
> But those are not the complete required tuples for the given SQL statement.
> Isn't it ?!

They better be.  If you wanted the locked tuples, you shouldn't have
asked to skip them.

-- 
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] [PROPOSAL] DIAGNOSTICS = SKIPPED_ROW_COUNT

2015-10-14 Thread dinesh kumar

Hi Robert,

On Wed, Oct 14, 2015 at 12:56 PM, Robert Haas  wrote:

> On Tue, Oct 13, 2015 at 10:37 PM, dinesh kumar 
> wrote:
> > In an existing wait policies like WAIT(default) and NO WAIT,
> > one can be sure to determine(Using ROW_COUNT daignostics counter),
> > how many required tuples he processed in a transaction.
> > But this is not case when it comes to SKIP LOCKED.
>
> Sure it is.  You didn't process the ones that you skipped.  This is no
> different than if you say WHERE a = 5.  Depending on plan choice and
> table contents, you may have "skipped" a large number of rows where a
> != 5, or you may have skipped none at all.
>

Yes, True.

But, using SKIP LOCKED we may bypass the tuples where a = 5, If those were
locked by parallel operations.

>
> > In my view, SKIP LOCKED is a nice feature, which gives only the
> available OR
> > unlocked tuples.
> > But those are not the complete required tuples for the given SQL
> statement.
> > Isn't it ?!
>
> They better be.

Agreed.

>   If you wanted the locked tuples, you shouldn't have
> asked to skip them.
>

Kindly let me know if I am going in a wrong way.

I see this feature as an add on to do the parallel DML operations.
There won't be any problem, if operations are mutually exclusive.
I mean, each session operates on unique set of tuples.

In the above case, we don't even need of SKIP LOCKED wait policy.

But, when it comes to mutually depend operations, isn't it nice to provide,
how much were locked by the other sessions. OR atlest a HINT to the other
session like,

GET DIAGNOSTICS var = DID_I_MISS_ANYTHING_FROM_OTHER_SESSIONS;

I agree that, adding counter will take a performance hit.
Rather going to my actual proposal on providing the counter value,
isn't it good to provide a boolean type HINT, if we miss atleast a single
tuple.

Let me know your thoughts.

Thanks in advance.

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

-- 

Regards,
Dinesh
manojadinesh.blogspot.com

Re: [HACKERS] pam auth - add rhost item

2015-10-14 Thread kolo hhmow

Yes, you right - my mistake.
But editing pg_hba.conf with lot of entries is little inconveniet. When
using pam modules with backend database like postgresql/or whatever
is more efficient and convenient - this is whay among others I need pass
client ip to pam modules, and then to backend database for example.
So I'm waiting for comments from others.
Thanks.

On Wed, Oct 14, 2015 at 9:52 PM, Robert Haas  wrote:

> On Tue, Oct 13, 2015 at 4:12 PM, kolo hhmow  wrote:
> > Yes, sorry. I was in hurry when I posted this message.
> > I dont understand whay in CheckPAMAuth function only PAM_USER item is
> adding
> > to pam information before authenticate?
> > Wheter it would be a problem to set additional pam information like
> > PAM_RHOST which is very useful because we can use this item to restrict
> > access to this ip address.
> > I hope I'm more specific now and you will understand me.
> > Sorry, but I'm not native english speaker.
> > Patch in attachment, and link below to web-view on github:
> >
> https://github.com/grzsmp/postgres/commit/5e2b102ec6de27e786d627623dcb187e997609e4
>
> I don't personally know much about PAM, but if you want to restrict
> access by IP, you could do that in pg_hba.conf.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: [HACKERS] Getting sorted data from foreign server

On Wed, Oct 14, 2015 at 7:30 AM, Ashutosh Bapat
 wrote:
> The patch uses a factor of 1.1 (10% increase) to multiple the startup and
> total costs in fpinfo for unsorted data.
>
> This change has caused the plans for few queries in the test postgres_fdw to
> change. There is ORDER BY and LIMIT clause in the queries in postgres_fdw
> testcase to keep test outputs sane and consistent. These ORDER BY clauses
> are not being pushed down the foreign servers. I tried using values upto 2
> for this but still the foreign paths with pathkeys won over those without
> pathkeys.

That's great.  Seeing unnecessary sorts disappear from the regression
tests as a result of this patch is, IMHO, pretty cool.  But these
compiler warnings might also be related:

postgres_fdw.c:605:7: error: variable 'rows' is used uninitialized whenever 'if'
  condition is false [-Werror,-Wsometimes-uninitialized]
if (fpinfo->use_remote_estimate)
^~~
postgres_fdw.c:628:13: note: uninitialized use occurs here
  ...rows,
 ^~~~
postgres_fdw.c:605:3: note: remove the 'if' if its condition is always true
if (fpinfo->use_remote_estimate)
^~~~
postgres_fdw.c:596:15: note: initialize the variable 'rows' to silence this
  warning
double  rows;
^
 = 0.0
postgres_fdw.c:605:7: error: variable 'startup_cost' is used uninitialized
  whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
if (fpinfo->use_remote_estimate)
^~~
postgres_fdw.c:629:13: note: uninitialized use occurs here
  ...startup_cost,
 ^~~~
postgres_fdw.c:605:3: note: remove the 'if' if its condition is always true
if (fpinfo->use_remote_estimate)
^~~~
postgres_fdw.c:598:21: note: initialize the variable 'startup_cost' to silence
  this warning
Coststartup_cost;
^
 = 0.0
postgres_fdw.c:605:7: error: variable 'total_cost' is used uninitialized
  whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
if (fpinfo->use_remote_estimate)
^~~
postgres_fdw.c:630:13: note: uninitialized use occurs here
  ...total_cost,
 ^~
postgres_fdw.c:605:3: note: remove the 'if' if its condition is always true
if (fpinfo->use_remote_estimate)
^~~~
postgres_fdw.c:599:19: note: initialize the variable 'total_cost' to silence
  this warning
Costtotal_cost;
  ^
   = 0.0

At least some of those look like actual mistakes.

I would suggest that instead of the regression test you added, you
instead change one of the existing tests to order by a non-pushable
expression that produces the same final output ordering.  The revised
EXPLAIN output shows that the existing tests are already testing that
the sort is getting pushed down; we don't need another test for the
same thing.  Instead, let's adjust one of the tests so that we can
also show that we don't push down ORDER BY clauses when it would be
wrong to do so.  For example, suppose the user specifies a collation
in the ORDER BY clause.

appendOrderByClause could use a header comment, and its definition
line is more than 80 characters, so it should be wrapped.

DEFAULT_FDW_SORT_MULTIPLIER should have a one-line comment, like /* If
no remote estimates, assume a sort costs 10% extra */.  The comment
inside postgresGetForeignPaths shouldn't refer specifically to the 10%
value, in case we change it.  So maybe: "Assume sorting costs
something, so that we won't ask for sorted results when they aren't
useful, but not too much, so that remote sorts will look attractive
when the ordering is useful to us."  Note that "attractive" is missing
a "t" in the patch.

Do you need to ignore ec_has_volatile equivalence classes in
find_em_expr_for_rel?  I bet yes.

-- 
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] Patch: Implement failover on libpq connect level.

2015-10-14 Thread Victor Wagner

On 2015.08.18 at 07:18:50 +0300, Victor Wagner wrote:

> Rationale
> =
> 
> Since introduction of the WAL-based replication into the PostgreSQL, it is
> possible to create high-availability and load-balancing clusters.
> 
> However, there is no support for failover in the client libraries. So, only
> way to provide transparent for client application failover is IP address
> migration. This approach has some limitation, i.e. it requires that
> master and backup servers reside in the same subnet or may not be
> feasible for other reasons.
> 
> Commercial RDBMS, such as Oracle, employ more flexible approach. They
> allow to specify multiple servers in the connect string, so if primary
> server is not available, client library tries to connect to other ones.
> 
> This approach allows to use geographically distributed failover clusters
> and also is a cheap way to implement load-balancing (which is not
> possible with IP address migration).
> 

Attached patch which implements client library failover and
loadbalancing as was described in the proposal
<20150818041850.ga5...@wagner.pp.ru>.

This patch implements following fuctionality:

1. It is allowed to specify several hosts in the connect string, either
in URL-style (separated by comma) or in param=value form (several host
parameters).

2. Each host parameter can be accompanied with port specifier separated
by colon. Port, specified such way takes precedence of port parameter or
default port for this particular host only.

3. There is hostorder parameter with two possible valies 'sequential'
and 'random' (default sequential). 'parallel' hostorder described in the
proposal is not yet implemented in this version of patch.

4. In the URL-style connect string parameter loadBalanceHosts=true is
considered equal to 'hostorder=random' for compatibility with jdbc.

5. Added new parameter readonly=boolean. If this parameter is false (the
default) upon successful connection check is performed that backend is
not in the recovery state. If so, connection is not considered usable
and next host is tried.

6. Added new parameter falover_timeout. If no usable host is found and
this parameter is specified, hosts are retried cyclically until this
timeout expires. It gives some time for claster management software to
promote one of standbys to master if master fails. By default there is
no retries.

Some implementation notes:

1. Field  pghost in the PGconn structure now stores comma separated list
of hosts, which is parsed in the connectDBStart. So, expected results of 
some tests in src/interfaces/libpq/test are changed. 

2. Url with colon  but no port number after the host no more considered
valid.

3. With hostorder=random we have to seed libc random number gernerator.
Some care was taken to not to lose entropy if generator was
initialized by app before connection, and to ensure different random
values if several connections are made from same application in one
second (even in single thread). RNG is seeded by xor of current time,
random value from this rng before seeding (if it was seeded earlier, it
keeps entropy) and address of the connection structure. May be it worth
effort adding thread id or pid, but there is no portable way to doing
so, so it would need testing on all supported platform.

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 0ee018e..79a99ec 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -792,7 +792,7 @@ host=localhost port=5432 dbname=mydb connect_timeout=10

The general form for a connection URI is:

-postgresql://[user[:password]@][netloc][:port][/dbname][?param1=value1...]
+postgresql://[user[:password]@][netloc][:port][,netloc[:port]...][/dbname][?param1=value1...]

@@ -809,6 +809,7 @@ postgresql://localhost/mydb
 postgresql://user@localhost
 postgresql://user:secret@localhost
 postgresql://other@localhost/otherdb?connect_timeout=10application_name=myapp
+postgresql://node1,node2:5433,node3:4432,node4/mydb?hostorder=randomreadonly=1

 Components of the hierarchical part of the URI can also
 be given as parameters.  For example:
@@ -831,7 +832,9 @@ postgresql:///mydb?host=localhostport=5433

 For improved compatibility with JDBC connection URIs,
 instances of parameter ssl=true are translated into
-sslmode=require.
+sslmode=require and
+loadBalanceHosts=true  into
+hostorder=random.

@@ -841,6 +844,10 @@ postgresql:///mydb?host=localhostport=5433
 postgresql://[2001:db8::1234]/database

+   
+   There can be serveral host specifications, optionally accompanied
+   with port, separated by comma.
+   

 The host component is interpreted as described for the parameter PostgreSQL was built). On machines without
 Unix-domain sockets, the default is to connect to localhost.

+   
+   There can be more than one host parameter in
+   the connect string. In this case these hosts would be considered

Re: [HACKERS] Getting sorted data from foreign server

2015-10-14 Thread Ashutosh Bapat

PFA the patch with all the comments addressed.

On Tue, Oct 13, 2015 at 10:07 PM, Robert Haas  wrote:

> On Tue, Oct 13, 2015 at 3:29 AM, Ashutosh Bapat
>  wrote:
> >> - You consider pushing down ORDER BY if any prefix of the query
> >> pathkeys satisfy is_foreign_expr(), but that doesn't seem right to me.
> >> If the user says SELECT * FROM remotetab ORDER BY a, unsafe(a),
> >> ordering the result set by a doesn't help us much.  We've talked a few
> >> times about an incremental sort capability that would take a stream of
> >> tuples already ordered by one or more columns and sort each group by
> >> additional columns, but I don't think we have that currently.  Without
> >> that capability, I don't think there's much benefit in sorting by a
> >> prefix of the pathkeys.  I suspect that if we can't get them all, it's
> >> not worth doing.
> >
> > I somehow thought, we are using output, which is ordered by prefix of
> > pathkeys in Sort nodes. But as you rightly pointed out that's not the
> case.
> > Only complete pathkeys are useful.
>
> A truncated list of pathkeys is useful for merge joins, but not for
> toplevel ordering.
>

Ok. Taken care in the attached patch.


>
> >> - Right now, you have this code below the point where we bail out if
> >> use_remote_estimate is not set.  If we keep it like that, the comment
> >> needs updating.  But I suggest that we consider an ordered path even
> >> if we are not using remote estimates.  Estimate the sorted path to
> >> cost somewhat more than the unsorted path, so that we only choose that
> >> path if the sort actually benefits us.  I don't know exactly how to
> >> come up with a principled estimate given that we don't actually know
> >> whether the remote side will need an extra sort or not, but maybe a
> >> dumb estimate is still better than not trying a sorted path.
> >
> > I like that idea, although there are two questions
> > 1. How can we estimate cost of getting the data sorted? If there is an
> > appropriate index on foreign server we can get the data sorted at no
> extra
> > cost. If there isn't the cost of sorting is proportionate to NlogN where
> N
> > is the size of data. It seems unreasonable to arrive at the cost of
> sorting
> > by multiplying with some constant multiplier. Also, the constant
> multiplier
> > to the NlogN estimate depends heavily upon the properties of foreign
> server
> > e.g. size of memory available for sorting, disk and CPU speed etc. The
> last
> > two might have got factored into fdw_tuple_cost and fdw_startup_cost, so
> > that's probably taken care of. If the estimate we come up turns out to be
> > too pessimistic, we will not get sorted data even if that's the right
> thing
> > to do. If too optimistic, we will incur heavy cost at the time of
> execution.
> > Setting the cost estimate to some constant factor of NlogN would be too
> > pessimistic if there is an appropriate index on foreign server. Otherway
> > round if there isn't an appropriate index on foreign server.
> >
> > Even if we leave these arguments aside for a while, the question remains
> as
> > to what should be the constant factor 10% or 20% or 50% or 100% or
> something
> > else on top of the estimate for simple foreign table scan estimates (or
> > NlogN of that)? I am unable to justify any of these factors myself. What
> do
> > you say?
>
> I think we want to estimate the cost in such a way that we'll tend to
> pick the ordered path if it's useful, but skip it if it's not.  So,
> say we pick 10%.  That's definitely enough that we won't pick a remote
> sort when it's useless, but it's small enough that if a remote sort is
> useful, we will probably choose to do it.  I think that's what we
> want.  I believe we should err on the side of a small estimate because
> it's generally better to do as much work as possible on the remote
> side.  In some cases the sort may turn out to be free at execution
> time because the remote server was going to generate the results in
> that order anyway, and it may know that because of its own pathkeys,
> and thus be able to skip the explicit ordering step.
>

The patch uses a factor of 1.1 (10% increase) to multiple the startup and
total costs in fpinfo for unsorted data.

This change has caused the plans for few queries in the test postgres_fdw
to change. There is ORDER BY and LIMIT clause in the queries in
postgres_fdw testcase to keep test outputs sane and consistent. These ORDER
BY clauses are not being pushed down the foreign servers. I tried using
values upto 2 for this but still the foreign paths with pathkeys won over
those without pathkeys.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 697de60..25d8650 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -186,23 +186,26 @@ is_foreign_expr(PlannerInfo *root,

Re: [HACKERS] Dangling Client Backend Process

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.

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] remaining open items

On 2015-10-13 21:57:15 -0400, Robert Haas wrote:
> - Refactoring speculative insertion with unique indexes a little.
> Andres seems to think this shouldn't be an open issue, while Peter
> thinks maybe it should be, or at least that's my imperfect executive
> summary.  Heikki isn't sure he agrees with all of the changes.  I'm
> not very clear on whether this is a new feature, a bug fix,
> beautification, or something else.

> What would happen if we didn't do anything at all?

Nothing, really. It's essentially some code beautification. A worthwhile
goal, but certainly not a release blocker.

Greetings,

Andres Freund


-- 
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: pg_confcheck - syntactic & semantic validation of postgresql configuration files

On 2015-10-14 01:54:46 +0300, Amir Rohan wrote:
> Andres, please see upthread for quite a bit on what it doesn't do, and
> why having it in the server is both an advantages and a shortcoming.

As far as I have skimmed the thread it's only talking about shortcoming
in case it requires a running server. Which -C doesn't.

I don't think there's any fundamental difference between some external
binary parsing & validating the config file and the postgres binary
doing it. There *is* the question of the utility being able to to
process options from multiple major releases, but I don't think that's a
particularly worthwhile goal here.

Greetings,

Andres Freund

-- 
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] Support for N synchronous standby servers - take 2

2015-10-14 Thread Masahiko Sawada

Reply to multiple member.

> Hm. This is not much helpful in the case we especially mentioned
> upthread at some point with 2 data centers, first one has the master
> and a sync standby, and second one has a set of standbys. We need to
> be sure that the standby in DC1 acknowledges all the time, and we
> would only need to wait for one or more of them in DC2. I still
> believe that this is the main use case for this feature to ensure a
> proper failover without data loss if one data center blows away with a
> meteorite.

Yes, I think so too.
In such case, the idea I posted yesterday could handle by setting the
followings;
* s_r_method = 'quorum'
* s_s_names = 'tokyo, seattle'
* s_s_nums = 2
* application_name of the first standby, which is in DC1, is 'tokyo',
and application_name of other standbys, which are in DC2, is
'seattle'.

> And I guess that they manage standby nodes using a system catalog
> then, being able to change the state of a node from async to sync with
> something at SQL level? Is that right?

I think that's right.

>
> If we go that path, I think that we still would need an extra
> parameter to control the number of nodes that need to be taken from
> the set defined in s_s_names whichever of quorum or priority is used.
> Let's not forget that in the current configuration the first node
> listed in s_s_names and *connected* to the master will be used to
> acknowledge the commit.

Yeah, such parameter is needed. I've forgotten to consider that.

>
>
> Would it be better to just use a simple language instead of 3 different
> parameters?
>
> s_s_names = 2[X,Y,Z]  # 2 priority
> s_s_names = 1(A,B,C) # 1 quorum
> s_s_names = R,S,T # default behavior: 1 priorty?

I think that this means that we have choose dedicated language
approach instead of JSON format approach.
If we want to set multi sync replication more complexly, we would not
have no choice other than  improvement of dedicated language.

Regards,

--
Masahiko Sawada


-- 
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] More work on SortSupport for text - strcoll() and strxfrm() caching

2015-10-14 Thread Peter Geoghegan

On Wed, Oct 14, 2015 at 4:09 PM, Robert Haas  wrote:
> I wonder if it wouldn't be better to just add a separate Boolean
> indicating exactly the thing we care about.  This doesn't seem
> particularly easy to understand and verify.

I'm not really sure that that's an improvement. But I defer to you.

-- 
Peter Geoghegan


-- 
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] DIAGNOSTICS = SKIPPED_ROW_COUNT

On Wed, Oct 14, 2015 at 6:28 PM, dinesh kumar  wrote:
> I see this feature as an add on to do the parallel DML operations.
> There won't be any problem, if operations are mutually exclusive.
> I mean, each session operates on unique set of tuples.
>
> In the above case, we don't even need of SKIP LOCKED wait policy.
>
> But, when it comes to mutually depend operations, isn't it nice to provide,
> how much were locked by the other sessions. OR atlest a HINT to the other
> session like,
>
> GET DIAGNOSTICS var = DID_I_MISS_ANYTHING_FROM_OTHER_SESSIONS;
>
> I agree that, adding counter will take a performance hit.
> Rather going to my actual proposal on providing the counter value,
> isn't it good to provide a boolean type HINT, if we miss atleast a single
> tuple.

Suppose there are 5 locked rows and 5 unlocked rows in the heap and you do this:

select * from t1 for share skip locked limit 5

The Boolean you propose will be false if the first 5 rows in physical
order are locked, and otherwise it will be false.  But there's no
difference between those two scenarios from the perspective of the
application.  Here's another example:

with foo as (select * from t1 for share skip locked) select * from foo
where a = 2;

If foo contains any locked rows at all, this will return true,
regardless of whether a = 2.

It's true that, for a lot of normal-ish queries, LockRows is applied
late enough that your proposed Boolean would return the intended
answer.  But there are a bunch of exceptions, like the ones shown
above, and there might be more in the future.

-- 
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] pam auth - add rhost item

2015-10-14 Thread Euler Taveira


On 14-10-2015 17:35, kolo hhmow wrote:

Yes, but this is very ugly solution, becasue you have to restart
postgresql daemon each time you have added a new user.

>
Restart != Reload. You can even do it using SQL.


This solution which I propose is give an abbility to dinamicaly manage
user accounts without need to restart each time a user account entry has
change.

>
Why do you want to double restrict the access? We already have HBA. 
Also, you could complicate the management because you need to check two 
different service configurations to figure out why foo user can't log 
in. I'm not a PAM expert but my impression is that rhost is an optional 
item. Therefore, advise PAM users to use HBA is a way to not complicate 
the actual feature.



--
   Euler Taveira   Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento


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

[HACKERS] Fix unclear comments in tablecmds.c

2015-10-14 Thread Amit Langote

Came across a couple of unclear comments about functions returning
ObjectAddress. Attached fixes them.

Thanks,
Amit
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7668c9d..403582c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5673,7 +5673,7 @@ ATPrepDropColumn(List **wqueue, Relation rel, bool recurse, bool recursing,
 }
 
 /*
- * Return value is that of the dropped column.
+ * Return value is the address of the dropped column.
  */
 static ObjectAddress
 ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
@@ -10376,7 +10376,7 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel)
  * coninhcount and conislocal for inherited constraints are adjusted in
  * exactly the same way.
  *
- * Return value is the OID of the relation that is no longer parent.
+ * Return value is the address of the relation that is no longer parent.
  */
 static ObjectAddress
 ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode)

-- 
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] Table partition + join pushdown

2015-10-14 Thread Kouhei Kaigai

> -Original Message-
> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Taiki Kondo
> Sent: Thursday, October 08, 2015 5:28 PM
> To: Kyotaro HORIGUCHI
> Cc: Kaigai Kouhei(海外 浩平); Iwaasa Akio(岩浅 晃郎);
> pgsql-hackers@postgresql.org
> Subject: Re: [HACKERS] [Proposal] Table partition + join pushdown
> 
> Hello, Horiguchi-san.
> 
> Thank you for your comment.
> 
> > I got some warning on compilation on unused variables and wrong
> > arguemtn type.
> 
> OK, I'll fix it.
> 
> > I failed to have a query that this patch works on. Could you let
> > me have some specific example for this patch?
> 
> Please find attached.
> And also make sure that setting of work_mem is '64kB' (not 64MB).
> 
> If there is the large work_mem enough to create hash table for
> relation after appending, its cost may be better than pushed-down
> plan's cost, then planner will not choose pushed-down plan this patch makes.
> So, to make this patch working fine, work_mem size must be smaller than
> the hash table size for relation after appending.
> 
> > This patch needs more comments. Please put comment about not only
> > what it does but also the reason and other things for it.
> 
> OK, I'll put more comments in the code.
> But it will take a long time, maybe...
>
People (including me) can help. Even though your English capability
is not enough, it is significant to put intention of the code.

> > -- about namings
> >
> > Names for functions and variables are needed to be more
> > appropriate, in other words, needed to be what properly informs
> > what they are. The followings are the examples of such names.
> 
> Thank you for your suggestion.
> 
> I also think these names are not good much.
> I'll try to make names better , but it maybe take a long time...
> Of course, I will use your suggestion as reference.
> 
> > "added_restrictlist"'s widely distributed as many function
> > arguemnts and JoinPathExtraData makes me feel
> > dizzy..
> 
> "added_restrictinfo" will be deleted from almost functions
> other than try_join_pushdown() in next (v2) patch because
> the place of filtering using this info will be changed
> from Join node to Scan node and not have to place it
> into other than try_join_pushdown().
>
This restrictinfo intends to filter out obviously unrelated rows
in this join, due to the check constraint of other side of the join.
So, correct but redundant name is:
  restrictlist_to_drop_unrelated_rows_because_of_check_constraint

How about 'restrictlist_by_constraint' instead?

> > In make_restrictinfos_from_check_constr, the function returns
> > modified constraint predicates correspond to vars under
> > hashjoinable join clauses. I don't think expression_tree_mutator
> > is suitable to do that since it could allow unwanted result when
> > constraint predicates or join clauses are not simple OpExpr's.
> 
> Do you have any example of this situation?
> I am trying to find unwanted results you mentioned, but I don't have
> any results in this timing. I have a hunch that it will allow unwanted
> results because I have thought only about very simple situation for
> this function.
>
check_constraint_mutator makes modified restrictlist with relacing
Var node only when join clause is hash-joinable.
It implies  =  form, thus we can safely replace the
expression by the other side.

Of course, we still have cases we cannot replace expressions simply.
- If function (or function called by operators) has volatile attribute
  (who use volatile function on CHECK constraint of partitioning?)
- If it is uncertain whether expression returns always same result.
  (is it possible to contain SubLink in the constraint?)

I'd like to suggest to use white-list approach in this mutator routine.
It means that only immutable expression node are allowed to include
the modified restrictlist.

Things to do is:

check_constraint_mutator(...)
{
  if (node == NULL)
return NULL;
  if (IsA(node, Var))
  {
 :
  }
  else if (node is not obviously immutable)
  {
context->is_mutated = false;  <-- prohibit to make if expression
  }   contains uncertain node.
  return expression_tree_mutator(...)
}


> > Otherwise could you give me clear explanation on what it does?
> 
> This function transfers CHECK() constraint to filter expression by following
> procedures.
> (1) Get outer table's CHECK() constraint by using get_relation_constraints().
> (2) Walk through expression tree got in (1) by using expression_tree_mutator()
> with check_constraint_mutator() and change only outer's Var node to
> inner's one according to join clause.
> 
> For example, when CHECK() constraint of table A is "num % 4 = 0" and
> join clause between table A and B is "A.num = B.data",
> then we can get "B.data % 4 = 0" for filtering purpose.
> 
> This also accepts more complex join clause like "A.num = B.data * 2",
> then we can get "(B.data * 2) % 4 = 0".
> 
> In procedure (2), to decide

Re: [HACKERS] INSERT ... ON CONFLICT documentation clean-up patch

2015-10-14 Thread Peter Geoghegan

Hi,

On Wed, Oct 14, 2015 at 9:38 AM, Andres Freund  wrote:
> "do not work sensibly" imo doesn't sound very good in docs. Maybe
> something roughly along the lines of "are unlikely to work as expected
> as the on conflict action is only taken in case of unique violation on
> the target relation, not child relations". I'd remove the whole bit
> about triggers not having access to ON CONFLICT.

"work sensibly" already appears in the documentation a number of
times. I won't argue with you on this, though.

>> diff --git a/doc/src/sgml/ref/insert.sgml b/doc/src/sgml/ref/insert.sgml
>> index 8caf5fe..0794acb3 100644
>> --- a/doc/src/sgml/ref/insert.sgml
>> +++ b/doc/src/sgml/ref/insert.sgml
>> @@ -99,7 +99,8 @@ INSERT INTO > class="PARAMETER">table_name [ AS >
>> You must have INSERT privilege on a table in
>> order to insert into it.  If ON CONFLICT DO UPDATE is
>> -   present the UPDATE privilege is also required.
>> +   present, UPDATE privilege on the table is also
>> +   required.
>>
>
> Is this actually an improvement?

I wanted to clarify that we're still talking about table-level privilege.

>> @@ -161,10 +162,7 @@ INSERT INTO > class="PARAMETER">table_name [ AS >  
>>   
>>A substitute name for the target table. When an alias is provided, it
>> -  completely hides the actual name of the table.  This is particularly
>> -  useful when using ON CONFLICT DO UPDATE into a 
>> table
>> -  named excluded as that's also the name of the
>> -  pseudo-relation containing the proposed row.
>> +  completely hides the actual name of the table.
>>   
>>  
>> 
>
> Hm?

I didn't think it was useful to acknowledge the case where a table is
named "excluded". It's a case that matters, but there is no point in
acknowledging it in the documentation. Users will just look for a way
to alias it on the rare occasion when this happens.

> I'm not convinced it's an improvement to talk about arbiter indexes
> here.

Okay.

>> +   For
>> +   ON CONFLICT DO NOTHING, it is optional to
>> +   specify a conflict_target; when omitted,
>> +   conflicts with all usable constraints (and unique indexes) are
>> +   handled.  For ON CONFLICT DO UPDATE, a
>> +   conflict_target must be
>> +   provided.
>
> Yes, that's a bit clearer.

Cool.

>> Every time an insertion without ON CONFLICT
>> would ordinarily raise an error due to violating one of the
>> -   inferred (or explicitly named) constraints, a conflict (as in
>> -   ON CONFLICT) occurs, and the alternative action,
>> -   as specified by conflict_action is taken.
>> -   This happens on a row-by-row basis.
>> +   inferred constraints/indexes (or an explicitly named constraint), a
>> +   conflict (as in ON CONFLICT) occurs, and the
>> +   alternative action, as specified by
>> +   conflict_action is taken.  This happens on a
>> +   row-by-row basis.  Only NOT DEFERRABLE
>> +   constraints are supported as arbiters.
>>
>
> I'm inclined ot only add the "Only NOT DEFERRABLE 
> constraints are
> supported as arbiters." bit - the rest doesn't really seem to be an
> improvement?

I'm just trying to make clear that at this level, it doesn't matter
how the arbiter was selected. Glad you agree that this is a better
place to talk about deferrable constraints, though.

>>
>> @@ -425,17 +427,28 @@ INSERT INTO > class="PARAMETER">table_name [ AS > specified columns/expressions and, if specified, whose predicate
>> implies the 
>> index_predicate are chosen as arbiter indexes.  Note
>> -   that this means an index without a predicate will be used if a
>> -   non-partial index matching every other criteria happens to be
>> -   available.
>> +   that this means a unique index without a predicate will be inferred
>> +   (and used by ON CONFLICT as an arbiter) if such
>> +   an index satisfying every other criteria is available.
>>
>
> Hm. I agree that the "happens to be available" sounds a bit too
> casual. But I think the sentence was easier to understand for endusers
> before?

Not sure that it was. "Happens" creates the impression that it could
happen almost by mistake.

>> +   
>> +
>> + A unique index inference clause should be preferred over naming a
>> + constraint directly using ON CONFLICT ON
>> + CONSTRAINT 
>> + constraint_name.  Unique index inference is
>> + adaptable to nonsignificant changes in the available constraints.
>> + Furthermore, it is possible for more than one constraint and/or
>> + unique index to be inferred for the same statement.
>> +
>> +   
>
> I'd formulate this a more neutral. How something like "... inference
> ...

I don't follow.

> Why did you move the exclusion constraint bit? Isn't it more appropriate
> in the action section?

I wanted to de-emphasize it. Whatever section it's in, it isn't
particularly important. It's not obvious what DO UPDATE + exclusion
constraints even means.

>> Note that the effects of all

Re: [HACKERS] More work on SortSupport for text - strcoll() and strxfrm() caching

On Wed, Oct 14, 2015 at 7:05 PM, Peter Geoghegan  wrote:
> On Mon, Oct 12, 2015 at 12:31 PM, Peter Geoghegan  wrote:
>> I'll consider a more comprehensive fix.
>
> I attach a revised fix that considers the problem of misinterpreting
> the contents of the buffers in both directions.

I wonder if it wouldn't be better to just add a separate Boolean
indicating exactly the thing we care about.  This doesn't seem
particularly easy to understand and verify.

-- 
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] A bunch of regular-expression improvements

2015-10-14 Thread Tom Lane

Some of you probably wondered where the heck the recent flurry of activity
around regular expressions (eg, commits 9fe8fe9c9e, b63fc2877) came from.
The answer is that it was mostly driven by some fuzz testing that Greg
Stark reported to the PG security list: he found various random regexp
patterns that crashed the regex compiler and/or caused it to take
unreasonable amounts of time and memory. Some of this was already known;
we had such a report back at
http://www.postgresql.org/message-id/b6f6fd62f2624c4c9916ac0175d56d880ce20...@jenmbs01.ad.intershop.net
and there are various similar complaints in the Tcl bug tracker.

The conclusion of the security list was that we didn't want to treat
such problems as CVE-worthy security issues, because given the history
of that code, no security-conscious DBA would allow use of regexes coming
from untrustworthy sources. (I'm not 100% on board with that reasoning
personally, though I have to agree that the warning we added about it was
prudent.) But we did do our best to fix cases where regex compilation
might be uncancelable for long intervals or might recurse to the point
of stack overflow, which were both things Greg showed to be possible.
That left us with some pretty severe performance issues, but hopefully
nothing that use of a statement timeout can't deal with.

Anyway, I got interested in whether the performance issues could be dealt
with properly, and have spent probably more time than I should've with my
nose buried in the regex engine. I've found a number of things that are
indeed fixable, and the attached set of patches are the result.

The primary thing that we found out is that the patch originally applied
to fix CVE-2007-4772 (infinite loop in pullback) is broken: not only does
it not fix all related cases, but it is capable of causing an Assert
failure. Fortunately the assertion condition is harmless in production
builds, so we didn't need to treat that as a security issue either. But
it's still possible to make pullback() go into an infinite loop that will
be broken only by running into the NFA state count limit.

Some background: what the pullback() and pushfwd() routines are trying to
do is get rid of ^ and $ constraint arcs by pushing them to the start and
stop states respectively, or dropping them as unsatisfiable if they
can't be so pushed (for example, in "a^x" there is no possible way to
satisfy the ^ constraint). Likewise AHEAD and BEHIND constraints can be
pushed to the start and stop states or discarded as either certainly
satisfied or certainly not satisfied. However, this works only if there
are no loops of constraint arcs, otherwise the transformation just keeps
pushing the same constraint around the loop, generating more and more
states. The pattern that prompted CVE-2007-4772 was "($|^)*", which
gives rise to an NFA state with ^ and $ constraint arcs that lead to
itself. So the hack that was instituted to deal with that was just to
drop such arcs as useless, which they are. (Whether or not you follow
the arc, you end up in the same state you started in, and you don't
consume any input; so it's useless to follow the arc.) However, that only
fixes trivial single-state constraint loops. It is not difficult to
construct regexes with constraint loops connecting more than one state.
Here are some examples:
(^$)*
(^(?!aa)(?!bb)(?!cc))+
The latter can be extended to construct a loop of as many states as you
please.

Also, I mentioned that the previous patch could cause an assertion
failure; the pattern "(^)+^" causes that. This happens because the
way the patch was implemented was to remove single-state loop arcs
within pull() or push(), and that subtly breaks the API contract
of those functions. If the removed loop arc was the final outarc
of its source state, and the only other outarc was another constraint
that had been passed to pull(), then pull() would remove both arcs
and then conclude it could delete the source state as well. We'd
then come back to pullback() whose nexta variable is pointing at a
now-deleted arc. Fortunately freearc() sets the type of deleted arcs
to zero, so the loop in pullback() would do nothing with the deleted
arc and exit successfully --- but the assert() that's there would
notice that something was wrong.

I concluded that the only way to fix this was to get rid of the hacks in
pull/push and instead implement a general constraint-loop-breaking pass
before we attempt pullback/pushfwd.

In principle it's okay to break a loop of constraint arcs for the same
reason cited above: traversing all the way around the loop, even if it's
possible because all the constraints are satisfied at the current point of
the string, is pointless since you're back in the same NFA state without
having consumed any input characters. Therefore we can break the loop by
removing the constraint arc(s) between any two chosen loop states, as long
as we make sure that it's still possible to follow any legal

Re: [HACKERS] Patch: Implement failover on libpq connect level.

2015-10-14 Thread Shulgin, Oleksandr

On Wed, Oct 14, 2015 at 12:41 PM, Victor Wagner  wrote:

> On 2015.08.18 at 07:18:50 +0300, Victor Wagner wrote:
>
> > Rationale
> > =
> >
> > Since introduction of the WAL-based replication into the PostgreSQL, it
> is
> > possible to create high-availability and load-balancing clusters.
> >
> > However, there is no support for failover in the client libraries. So,
> only
> > way to provide transparent for client application failover is IP address
> > migration. This approach has some limitation, i.e. it requires that
> > master and backup servers reside in the same subnet or may not be
> > feasible for other reasons.
> >
> > Commercial RDBMS, such as Oracle, employ more flexible approach. They
> > allow to specify multiple servers in the connect string, so if primary
> > server is not available, client library tries to connect to other ones.
> >
> > This approach allows to use geographically distributed failover clusters
> > and also is a cheap way to implement load-balancing (which is not
> > possible with IP address migration).
> >
>
> Attached patch which implements client library failover and
> loadbalancing as was described in the proposal
> <20150818041850.ga5...@wagner.pp.ru>.
>
> This patch implements following fuctionality:
>
> 1. It is allowed to specify several hosts in the connect string, either
> in URL-style (separated by comma) or in param=value form (several host
> parameters).
>

So what exactly would happen with this command: PGHOST=host1 psql -h host2

Or this one: PGHOST=host1 psql host=host2

What about service files?

2. Each host parameter can be accompanied with port specifier separated
> by colon. Port, specified such way takes precedence of port parameter or
> default port for this particular host only.
>
> 3. There is hostorder parameter with two possible valies 'sequential'
> and 'random' (default sequential). 'parallel' hostorder described in the
> proposal is not yet implemented in this version of patch.
>
> 4. In the URL-style connect string parameter loadBalanceHosts=true is
> considered equal to 'hostorder=random' for compatibility with jdbc.
>
> 5. Added new parameter readonly=boolean. If this parameter is false (the
> default) upon successful connection check is performed that backend is
> not in the recovery state. If so, connection is not considered usable
> and next host is tried.
>
> 6. Added new parameter falover_timeout. If no usable host is found and
> this parameter is specified, hosts are retried cyclically until this
> timeout expires. It gives some time for claster management software to
> promote one of standbys to master if master fails. By default there is
> no retries.
>
> Some implementation notes:
>
> 1. Field  pghost in the PGconn structure now stores comma separated list
> of hosts, which is parsed in the connectDBStart. So, expected results of
> some tests in src/interfaces/libpq/test are changed.
>

 trying postgresql://[::1]:12345/db
-dbname='db' host='::1' port='12345' (inet)
+dbname='db' host='[::1]:12345' (inet)

Such a change doesn't look really nice to me.


> 2. Url with colon  but no port number after the host no more considered
> valid.
>

We could live with that, but is there a good reason for backward
compatibility break in this case?

--
Alex

[HACKERS] pg_restore cancel TODO

2015-10-14 Thread Jeff Janes

I've added the TODO item:

When pg_upgrade -j ... is interrupted (for example, ctrl-C from the
keyboard) make it cancel the children processes.

The context where this arises is that I want to populate data into a new
installation compiled with a patch under review, but immediately get error
messages indicating I forgot to install a required extension.  I hit ctrl-C
so I can fix the problem, but it keeps running anyway.

Cheers,

Jeff

Re: [HACKERS] Proposal: pg_confcheck - syntactic & semantic validation of postgresql configuration files

2015-10-14 Thread David Fetter

On Wed, Oct 14, 2015 at 01:52:21AM +0300, Amir Rohan wrote:
> On 10/14/2015 01:12 AM, Alvaro Herrera wrote:
> > Amir Rohan wrote:
> >> On 10/14/2015 12:14 AM, Alvaro Herrera wrote:
> >>> Amir Rohan wrote:
> >>>
>  I've been considering that. Reusing the parser would ensure no errors
>  are introduces by having a different implementation, but on the other
>  hand involving the pg build in installation what's intended as a
>  lightweight, independent tool would hurt.
>  Because it's dubious whether this will end up in core, I'd like
>  "pip install pg_confcheck" to be all that is required.
> >>>
> >>> Maybe just compile a single file in a separate FRONTEND environment?
> >>
> >> You mean refactoring the postgres like rhass means? could you elaborate?
> >>
> >> I believe most people get pg as provided by their distro or PaaS,
> >> and not by compiling it.
> > 
> > I mean the utility would be built by using a file from the backend
> > source, just like pg_xlogdump does.  We have several such cases.
> > I don't think this is impossible to do outside core, either.
> 
> I've considered "vendoring", but it seems like enough code surgery
> be involved to make this very dubious "reuse". The language is simple
> enough that writing a parser from scratch isn't a big deal hard, and
> there doesn't seem much room for divergent parsing either.

Such room as there is seems worth eliminating if possible.  There's
even a formal name for this issue, which attackers can use, although
the implications as a source of subtle bugs in the absence of an
attacker seem more pertinent right now.

https://www.google.com/?q=parse%20tree%20differential%20attack

> So, the only question is whether reusing the existing parser will
> brings along some highly useful functionality beyond an AST and
> a battle-tested validator for bools, etc'. I'm not ruling anything
> out yet, though.

I suspect that having a single source parser, however painful now,
will pay large dividends later.

Cheers,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
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: pg_confcheck - syntactic & semantic validation of postgresql configuration files

On 10/14/2015 07:41 PM, David Fetter wrote:
>> On Wed, Oct 14, 2015 at 01:52:21AM +0300, Amir Rohan wrote:

>>
>> I've considered "vendoring", but it seems like enough code surgery
>> be involved to make this very dubious "reuse". The language is simple
>> enough that writing a parser from scratch isn't a big deal hard, and
>> there doesn't seem much room for divergent parsing either.
> 
> Such room as there is seems worth eliminating if possible.  

IMO, not If it means the tool will thus dodge minor potential
for harmless bugs, but becomes far more difficult to install
in the process.

> There's even a formal name for this issue, which attackers can use, although
> the implications as a source of subtle bugs in the absence of an
> attacker seem more pertinent right now.
> 
> https://www.google.com/?q=parse%20tree%20differential%20attack
> 

Interesting, but the security implications of a "parser attack" or
indeed different parsing behavior in some corner cases, are roughly
nil in this case.

>> So, the only question is whether reusing the existing parser will
>> brings along some highly useful functionality beyond an AST and
>> a battle-tested validator for bools, etc'. I'm not ruling anything
>> out yet, though.
> 
> I suspect that having a single source parser, however painful now,
> will pay large dividends later.
> 

I don't think anything is more important for a productivity tool then
being easily installed and easy to use.
It's a fact of life that all software will have bugs. If they matter,
you fix them.

Still, if there are more concrete benefits to adapting the parser, such
as batteries-included features I'm simply unaware of, I'd love to hear
about it.

Thanks,
Amir

-- 
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

2015-10-14 Thread Tom Lane

Alvaro Herrera  writes:
> Andres Freund wrote:
>> On 2015-10-14 14:19:40 -0300, Alvaro Herrera wrote:
>>> I think we could continue to have the parameter except that it throws an
>>> error if you try to set it to something other than 0.

>> That'll make it hard to ever remove it tho.

> What would you recommend then?  Forcing the user to specify the version
> before the connection is established is not nice.

Yeah.  I thought about telling Shay to set the variable after establishing
the connection, but there's a problem with that: if the user issues RESET
ALL then his setting would go away.  (IIRC, settings established in the
connection packet are considered to be what to reset to; but a SET sent
just after connection would not be.)

The only other alternative is to make a second connection attempt if the
first fails, which is pretty messy.

If we think it's legit for npgsql to try to force renegotiation off,
then we have to give pretty serious consideration to putting the variable
back as Alvaro suggests.

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] Database schema diff

2015-10-14 Thread Michal Novotny

Hi,

thanks a lot for your reply, unfortunately it's not working at all, I
run it as:

# java -jar apgdiff-2.4.jar  

But it's stuck on the futex wait so unfortunately it didn't work at all.

Thanks for the reply anyway,
Michal


On 10/14/2015 01:53 PM, Иван Фролков wrote:
>> I would like to ask you whether is there any tool to be able to compare
>> database schemas ideally no matter what the column order is or to dump
>> database table with ascending order of all database columns.
> 
> Take a look a tool called apgdiff http://apgdiff.com/
> Its development seems suspended, but it is still useful tool, except cases 
> with new features etc.
> Anyway, you could find bunch of forks at the github - I did support for 
> instead of triggers, other people did another options and so on
> 
> 


-- 
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] Database schema diff

2015-10-14 Thread Michal Novotny

I have to admit I was having the same idea few years ago however I never
got to implement it, nevertheless I should mount 2 trees for diff
comparison, isn't that correct?

I mean to mount  as /mnt/dumps/old and  Few years ago I developed a tool called fsgateway
> (https://github.com/mk8/fsgateway) that show metadata (table, index,
> sequences, view) as normal files using fuse.
> In this way to yout can get differences between running db instance
> using diff, meld or what do you prefear.
> 
> Unfortunally at the moment not all you need is supported, yet.
> 
> Best regards
> 
> P.S. I think that this is the wrong list for questione like this one.
> 
> On Wed, Oct 14, 2015 at 10:26 AM, Shulgin, Oleksandr
> > wrote:
> 
> On Tue, Oct 13, 2015 at 5:48 PM, Michal Novotny
> >
> wrote:
> 
> Hi guys,
> 
> I would like to ask you whether is there any tool to be able to
> compare
> database schemas ideally no matter what the column order is or
> to dump
> database table with ascending order of all database columns.
> 
> For example, if I have table (called table) in schema A and in
> schema B
> (the time difference between is 1 week) and I would like to
> verify the
> column names/types matches but the order is different, i.e.:
> 
> Schema A (2015-10-01) |  Schema B (2015-10-07)
>   |
> id int|  id int
> name varchar(64)  |  name varchar(64)
> text text |  description text
> description text  |  text text
> 
> Is there any tool to compare and (even in case above) return
> that both
> tables match? Something like pgdiff or something?
> 
> This should work for all schemas, tables, functions, triggers
> and all
> the schema components?
> 
> 
> I've used pg_dump --split for this purpose a number of times (it
> requires patching pg_dump[1]).
> 
> The idea is to produce the two database's schema dumps split into
> individual files per database object, then run diff -r against the
> schema folders.  This worked really well for my purposes.
> 
> This will however report difference in columns order, but I'm not
> really sure why would you like to ignore that.
> 
> --
> Alex
> 
> [1] 
> http://www.postgresql.org/message-id/AANLkTikLHA2x6U=q-t0j0ys78txhfmdtyxjfsrsrc...@mail.gmail.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] Proposal: pg_confcheck - syntactic & semantic validation of postgresql configuration files

On 10/14/2015 01:30 PM, Andres Freund wrote:
> On 2015-10-14 01:54:46 +0300, Amir Rohan wrote:
>> Andres, please see upthread for quite a bit on what it doesn't do, and
>> why having it in the server is both an advantages and a shortcoming.
> 
> As far as I have skimmed the thread it's only talking about shortcoming
> in case it requires a running server. Which -C doesn't.
> 

That's helpful, no one has suggested -C yet. This was new to me.

Here's what the usage says:

Usage:
  postgres [OPTION]...

Options:
  -C NAMEprint value of run-time parameter, then exit

```

P1. The fact that -C will warn about bad values when you query for an
unrelated name (as it requires you to do) is cunningly and successfully
elided. I expected to have to check every one individually. How about a
no-argument version of "-C"?

P2. Next, that it will print the value of a "run-time" parameter,
without an actual process is another nice surprise, which I wouldn't
have guessed.

The error messages are all one could wish for:

LOG:  invalid value for parameter "wal_level": "archive2"
HINT:  Available values: minimal, archive, hot_standby, logical.
LOG:  parameter "fsync" requires a Boolean value

*and* it prints everything it finds wrong, not stopping at the first
one. very nice.

it does fail the "dependent options" test:
$ postgres -C "archive_mode"
on
$ postgres -C wal_level
minimal

no errors, great, let's try it:
$ pg_ctl restart

FATAL:  WAL archival cannot be enabled when wal_level is "minimal"

> I don't think there's any fundamental difference between some external
> binary parsing & validating the config file and the postgres binary
> doing it. 

-C does a much better job then pg_file_settings for this task, I agree.
Still, it doesn't answer the already mentioned points of:
1) You need a server (binary, not a running server) to check a config
(reasonable), with a version matching what you'll run on (again, reasonable)
2) It doesn't catch more complicated problems like dependent options.
3) It doesn't catch bad ideas, only errors.
4) You can't easily extend the checks performed, without forking
postgres or going through the (lengthy, rigorous) cf process.
5) Because it checks syntax only, you don't get the benefits of having
an official place for the community to easily contribute rules that
warn you against config pitfalls, so that all users benefits.
See my OP for real-world examples and links about this problem.

> There *is* the question of the utility being able to to
> process options from multiple major releases, but I don't think that's a
> particularly worthwhile goal here.
> 

For one thing, I think it's been made clear that either few people know
about -C or few use it as part of their usual workflow. That in itself
is an issue. Any bloggers reading this?

Next, you may not agree semantic checks/advice is useful. I only
agree it's easy to dismiss until it saves your (i.e. a user's) ass
that first time.

I would *highly* recommend the talk mentioned in the OP:

"...Lag - What's Wrong with My Slave?"  (Samantha Billington, 2015)
https://youtu.be/If22EwtCFKc

Not just for the concrete examples (*you* know those by heart), but to
really hear the frustration in a real production user's voice when they
deploy pg in production, hit major operational difficulties, spend lots
of time and effort trying to root-cause and finally realize they simply
needed to "turn on FOO". Most of these problem can be caught very easily
with a conf linter.

Amir

-- 
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] Database schema diff

2015-10-14 Thread Michal Novotny

Hi Christopher,

thanks a lot for your suggestion however I need to run against dump
files so it's useless for me.

Thanks anyway,
Michal


On 10/13/2015 07:23 PM, Christopher Browne wrote:
> On 13 October 2015 at 11:48, Michal Novotny
> > wrote:
> 
> Hi guys,
> 
> I would like to ask you whether is there any tool to be able to compare
> database schemas ideally no matter what the column order is or to dump
> database table with ascending order of all database columns.
> 
> For example, if I have table (called table) in schema A and in schema B
> (the time difference between is 1 week) and I would like to verify the
> column names/types matches but the order is different, i.e.:
> 
> Schema A (2015-10-01) |  Schema B (2015-10-07)
>   |
> id int|  id int
> name varchar(64)  |  name varchar(64)
> text text |  description text
> description text  |  text text
> 
> Is there any tool to compare and (even in case above) return that both
> tables match? Something like pgdiff or something?
> 
> This should work for all schemas, tables, functions, triggers and all
> the schema components?
> 
> Also, is there any tool to accept 2 PgSQL dump files (source for
> pg_restore) and compare the schemas of both in the way above?
> 
> Thanks a lot!
> Michal
> 
> 
> I built a tool I call "pgcmp", which is out on GitHub
> 
> 
> The one thing that you mention that it *doesn't* consider is the
> ordering of columns.
> 
> It would not be difficult at all to add that comparison; as simple as adding
> an extra capture of table columns and column #'s.  I'd be happy to consider
> adding that in.
> 
> Note that pgcmp expects the database to be captured as databases; it
> pulls data
> from information_schema and such.  In order to run it against a pair of
> dumps,
> you'd need to load those dumps into databases, first.
> -- 
> When confronted by a difficult problem, solve it by reducing it to the
> question, "How would the Lone Ranger handle this?"


-- 
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: pg_confcheck - syntactic & semantic validation of postgresql configuration files

On 2015-10-14 16:17:55 +0300, Amir Rohan wrote:
> it does fail the "dependent options" test:
> $ postgres -C "archive_mode"
> on
> $ postgres -C wal_level
> minimal

Yea, because that's currently evaluated outside the config
mechanism. It'd imo would be good to change that independent of this
thread.

> 5) Because it checks syntax only, you don't get the benefits of having
> an official place for the community to easily contribute rules that
> warn you against config pitfalls, so that all users benefits.
> See my OP for real-world examples and links about this problem.

I don't think we as a community want to do that without review
mechanisms in place, and I personally don't think we want to add
separate processes for this.

Greetings,

Andres Freund

-- 
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] Typo in replorigin_sesssion_origin (9.5+)

Craig Ringer wrote:
> Hi all
> 
> Before 9.5 goes final, lets change replorigin_sesssion_origin and
> replorigin_sesssion_origin_lsn to remove the extra 's'.

Hmm?  I already fixed this two weeks ago in
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=aea76d128ad85f38aa0f4255fb9d46d95b835755

-- 
Á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] Foreign join pushdown vs EvalPlanQual

2015-10-14 Thread Kouhei Kaigai

> -Original Message-
> From: Kyotaro HORIGUCHI [mailto:horiguchi.kyot...@lab.ntt.co.jp]
> Sent: Wednesday, October 14, 2015 4:40 PM
> To: Kaigai Kouhei(海外 浩平)
> Cc: fujita.ets...@lab.ntt.co.jp; pgsql-hackers@postgresql.org;
> shigeru.han...@gmail.com; robertmh...@gmail.com
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
> 
> Hello,
> 
> At Wed, 14 Oct 2015 03:07:31 +, Kouhei Kaigai  wrote
> in <9a28c8860f777e439aa12e8aea7694f801157...@bpxm15gp.gisp.nec.co.jp>
> > > I noticed that the approach using a column to populate the foreign
> > > scan's slot directly wouldn't work well in some cases.  For example,
> > > consider:
> > >
> > > SELECT * FROM verysmall v LEFT JOIN (bigft1 JOIN bigft2 ON bigft1.x =
> > > bigft2.x) ON v.q = bigft1.q AND v.r = bigft2.r FOR UPDATE OF v;
> > >
> > > The best plan is presumably something like this as you said before:
> > >
> > > LockRows
> > > -> Nested Loop
> > > -> Seq Scan on verysmall v
> > > -> Foreign Scan on bigft1 and bigft2
> > >  Remote SQL: SELECT * FROM bigft1 JOIN bigft2 ON bigft1.x =
> > > bigft2.x AND bigft1.q = $1 AND bigft2.r = $2
> > >
> > > Consider the EvalPlanQual testing to see if the updated version of a
> > > tuple in v satisfies the query.  If we use the column in the testing, we
> > > would get the wrong results in some cases.
> 
> I have a basic (or maybe silly) qustion. Is it true that the
> join-inner (the foreignscan in the example) is re-executed with
> the modified value of v.r?  I observed for a join case among only
> local tables that previously fetched tuples for the inner are
> simplly reused regardless of join types. Even when a refetch
> happens (I haven't confirmed but it would occur in the case of no
> security quals), the tuple is pointed by ctid so the re-join
> between local and remote would fail. Is this wrong?
>
Let's dive into ExecNestLoop().
Once nl_NeedNewOuter is true, ExecProcNode(outerPlan) is called then
ExecReScan(innerPlan) is called with new param-info delivered from the
outer-tuple.

nl_NeedNewOuter is reset just after ExecProcNode(outerPlan), then
it is set once outer-tuple is needed again when inner-scan reached
to end of the relation, or found a tuple on semi-join.
In case of semi-join returned a joined-tuple then EPQ recheck is
applied, it can call ExecProcNode(outerPlan) and reset inner-plan
state.

It is what I can say from the existing code.
I doubt whether the behavior is right on EPQ rechecks. The above scenario
introduces the inner-relation (verysmall) is updated by the concurrent
session, thus param-info has to be updated.

However, it does not looks to me the implementation pays attention here.
If ExecNestLoop() is called under the EPQ recheck context, it needs to
call ExecProcNode() towards both of outer and inner plan to ensure the
visibility of joined-tuple towards the latest status.
Of course, underlying scan plans for base relations never make advance
the scan pointer. It just returns a tuple in EPQ slot, then I want
ExecNestLoop() to evaluate whether these tuples satisfies the join-clause.


> > In this case, does ForeignScan have to be reset prior to ExecProcNode()?
> > Once ExecReScanForeignScan() gets called by ExecNestLoop(), it marks EPQ
> > slot is invalid. So, more or less, ForeignScan needs to kick the remote
> > join again based on the new parameter come from the latest verysmall tuple.
> > Please correct me, if I don't understand correctly.
> 
> So, no rescan would happen for the cases, I think. ReScan seems
> to be kicked only for the new(next) outer tuple that causes
> change of parameter, but not kicked for EPQ. I might take you
> wrongly..
> 
> > In case of unparametalized ForeignScan case, the cached join-tuple work
> > well because it is independent from verysmall.
> 
> 
> > Once again, if FDW driver is responsible to construct join-tuple from
> > the base relation's tuple cached in EPQ slot, this case don't need to
> > kick remote query again, because all the materials to construct join-
> > tuple are already held locally. Right?
> 
> It is definitely right and should be doable. But I think the
> point we are argueing here is what is the desirable behavior.
>
In case of scanrelid==0, expectation to ForeignScan/CustomScan is to
behave as if local join exists here. It requires ForeignScan to generate
joined-tuple as a result of remote join, that may contains multiple junk
TLEs to carry whole-var references of base foreign tables.
According to the criteria, the desirable behavior is clear as below:

1. FDW/CSP picks up base relation's tuple from the EPQ slots.
   It shall be setup by whole-row reference if earlier row-lock semantics,
   or by RefetchForeignRow if later row-lock semantics.

2. Fill up ss_ScanTupleSlot according to the xxx_scan_tlist.
   We may be able to provide a common support function here, because this
   list keeps relation between a particular attribute of the joined-tuple
   and its source column.

3.

Re: [HACKERS] Proposal: pg_confcheck - syntactic & semantic validation of postgresql configuration files

On 10/14/2015 04:24 PM, Andres Freund wrote:
> On 2015-10-14 16:17:55 +0300, Amir Rohan wrote:
>> it does fail the "dependent options" test:
>> $ postgres -C "archive_mode"
>> on
>> $ postgres -C wal_level
>> minimal
> 
> Yea, because that's currently evaluated outside the config
> mechanism. It'd imo would be good to change that independent of this
> thread.
> 

I agree.

>> 5) Because it checks syntax only, you don't get the benefits of having
>> an official place for the community to easily contribute rules that
>> warn you against config pitfalls, so that all users benefits.
>> See my OP for real-world examples and links about this problem.
> 
> I don't think we as a community want to do that without review
> mechanisms in place, and I personally don't think we want to add
> separate processes for this.
> 

That's what "contribute" means in my book. I'm getting mixed signals
about what the "community" wants. I certainly think if adding rules
involves modifying the postgres server code, that is far too high
a bar and no one will.

I'm not sure what you mean by "separate process". My original
pitch was to have this live in core or contrib, and no one wanted
it. If you don't want it in core, but people thinks its a good idea to
have (with review), what would you suggest?

Amir

-- 
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] More work on SortSupport for text - strcoll() and strxfrm() caching

2015-10-14 Thread Peter Geoghegan

On Mon, Oct 12, 2015 at 12:31 PM, Peter Geoghegan  wrote:
> I'll consider a more comprehensive fix.

I attach a revised fix that considers the problem of misinterpreting
the contents of the buffers in both directions.

Thanks
-- 
Peter Geoghegan
From c2d4558df3ce93b939fa319dd6e0735400833df0 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan 
Date: Mon, 12 Oct 2015 17:13:28 -0700
Subject: [PATCH] Correctly distinguish the contents of text buffers

Avoid confusing an original string with a strxfrm() buffer, or
vice-versa.  This could previously occur in the event of interleaving of
authoritative comparisons with conversions to abbreviated keys.  Such
interleaving is possible with external sorts.
---
 src/backend/utils/adt/varlena.c | 35 +++
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index d545c34..c8574f0 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -64,7 +64,7 @@ typedef struct
 	int			buflen2;
 	int			last_len1;		/* Length of last buf1 string/strxfrm() blob */
 	int			last_len2;		/* Length of last buf2 string/strxfrm() blob */
-	int			last_returned;	/* Last comparison result (cache) */
+	int			last_returned;	/* Last comparison result/caching sentinel */
 	bool		collate_c;
 	hyperLogLogState abbr_card; /* Abbreviated key cardinality state */
 	hyperLogLogState full_card; /* Full key cardinality state */
@@ -1843,10 +1843,17 @@ btsortsupport_worker(SortSupport ssup, Oid collid)
 #endif
 		/*
 		 * To avoid somehow confusing a strxfrm() blob and an original string
-		 * within bttextfastcmp_locale(), initialize last returned cache to a
+		 * within bttextfastcmp_locale(), initialize last_returned cache to a
 		 * sentinel value.  A platform-specific actual strcoll() return value
-		 * of INT_MIN seems unlikely, but if that occurs it will not cause
-		 * wrong answers.
+		 * of INT_MIN seems unlikely, but will be immediately changed to -1 to
+		 * remove the possibility of wrong answers.
+		 *
+		 * Comparisons that attempt to reuse last_returned may be interleaved
+		 * with conversion calls.  Frequently, conversions and comparisons are
+		 * batched into two distinct phases, but the correctness of caching
+		 * cannot hinge upon this.  For comparison caching we only trust the
+		 * cache when last_returned != INT_MIN, while for strxfrm() caching we
+		 * only trust the cache when last_returned == INT_MIN.
 		 */
 		tss->last_returned = INT_MIN;
 		tss->collate_c = collate_c;
@@ -1931,9 +1938,9 @@ bttextfastcmp_locale(Datum x, Datum y, SortSupport ssup)
 	if (len1 == len2 && memcmp(a1p, a2p, len1) == 0)
 	{
 		/*
-		 * No change in buf1 or buf2 contents, so avoid changing last_len1 or
-		 * last_len2.  Existing contents of buffers might still be used by next
-		 * call.
+		 * No change in buf1 or buf2 contents, so avoid changing last_len1,
+		 * last_len2, or last_returned.  Existing contents of buffers might
+		 * still be used by next call.
 		 */
 		result = 0;
 		goto done;
@@ -2005,7 +2012,12 @@ bttextfastcmp_locale(Datum x, Datum y, SortSupport ssup)
 	if (result == 0)
 		result = strcmp(tss->buf1, tss->buf2);
 
-	/* Cache result, perhaps saving an expensive strcoll() call next time */
+	/*
+	 * Cache result, perhaps saving an expensive strcoll() call next time.
+	 * Interpret a strcoll() return value that happens to be the sentinel value
+	 * INT_MIN as -1.
+	 */
+	result = Max(-1, result);
 	tss->last_returned = result;
 done:
 	/* We can't afford to leak memory here. */
@@ -2091,6 +2103,7 @@ bttext_abbrev_convert(Datum original, SortSupport ssup)
 
 		/* Might be able to reuse strxfrm() blob from last call */
 		if (tss->last_len1 == len &&
+			tss->last_returned == INT_MIN &&
 			memcmp(tss->buf1, authoritative_data, len) == 0)
 		{
 			memcpy(pres, tss->buf2, Min(sizeof(Datum), tss->last_len2));
@@ -2173,6 +2186,12 @@ bttext_abbrev_convert(Datum original, SortSupport ssup)
 
 done:
 	/*
+	 * Set last_returned to sentinel value to indicate that buffers store
+	 * strxfrm() original string and blob
+	 */
+	tss->last_returned = INT_MIN;
+
+	/*
 	 * Byteswap on little-endian machines.
 	 *
 	 * This is needed so that bttextcmp_abbrev() (an unsigned integer 3-way
-- 
1.9.1


-- 
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

On Wed, Oct 14, 2015 at 12:30 PM, Amit Kapila  wrote:
>> - I continue to think GetParallelShmToc is the wrong approach.
>> Instead, each time ExecParallelInitializeDSM or
>> ExecParallelInitializeDSM calls a nodetype-specific initialized
>> function (as described in the previous point), have it pass d->pcxt as
>> an argument.  The node can get the toc from there if it needs it.  I
>> suppose it could store a pointer to the toc in its scanstate if it
>> needs it, but it really shouldn't.  Instead, it should store a pointer
>> to, say, the ParallelHeapScanDesc in the scanstate.
>
> How will this idea work for worker backend.  Basically in worker
> if want something like this to work, toc has to be passed via
> QueryDesc to Estate and then we can retrieve ParallelHeapScanDesc
> during PartialSeqScan initialization (ExecInitPartialSeqScan).
> Do you have something else in mind?

Good question.  I think when the worker starts up it should call yet
another planstate-walker, e.g. ExecParallelInitializeWorker, which can
call nodetype-specific functions for parallel-aware nodes and give
each of them a chance to access the toc and store a pointer to their
parallel shared state (ParallelHeapScanDesc in this case) in their
scanstate.  I think this should get called from ParallelQueryMain
after ExecutorStart and before ExecutorRun:
ExecParallelInitializeWorker(queryDesc->planstate, toc).

-- 
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] Parallel Seq Scan