Re: [HACKERS] CustomScan under the Gather node?

[Kouhei Kaigai]

> If I would make a proof-of-concept patch with interface itself, it
> seems to me file_fdw may be a good candidate for this enhancement.
> It is not a field for postgres_fdw.
>
The attached patch is enhancement of FDW/CSP interface and PoC feature
of file_fdw to scan source file partially. It was smaller enhancement
than my expectations.

It works as follows. This query tried to read 20M rows from a CSV file,
using 3 background worker processes.

postgres=# set max_parallel_degree = 3;
SET
postgres=# explain analyze select * from test_csv where id % 20 = 6;
  QUERY PLAN

 Gather  (cost=1000.00..194108.60 rows=94056 width=52)
 (actual time=0.570..19268.010 rows=200 loops=1)
   Number of Workers: 3
   ->  Parallel Foreign Scan on test_csv  (cost=0.00..183703.00 rows=94056 
width=52)
  (actual time=0.180..12744.655 rows=50 
loops=4)
 Filter: ((id % 20) = 6)
 Rows Removed by Filter: 950
 Foreign File: /tmp/testdata.csv
 Foreign File Size: 1504892535
 Planning time: 0.147 ms
 Execution time: 19330.201 ms
(9 rows)


I'm not 100% certain whether this implementation of file_fdw is reasonable
for partial read, however, the callbacks located on the following functions
enabled to implement a parallel-aware custom logic based on the coordination
information.

> * ExecParallelEstimate
> * ExecParallelInitializeDSM
> * ExecParallelInitializeWorker

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 

> -Original Message-
> From: Kaigai Kouhei(海外 浩平)
> Sent: Thursday, January 28, 2016 9:33 AM
> To: 'Robert Haas'
> Cc: pgsql-hackers@postgresql.org
> Subject: Re: [HACKERS] CustomScan under the Gather node?
> 
> > On Tue, Jan 26, 2016 at 1:30 AM, Kouhei Kaigai  wrote:
> > > What enhancement will be necessary to implement similar feature of
> > > partial seq-scan using custom-scan interface?
> > >
> > > It seems to me callbacks on the three points below are needed.
> > > * ExecParallelEstimate
> > > * ExecParallelInitializeDSM
> > > * ExecParallelInitializeWorker
> > >
> > > Anything else?
> > > Does ForeignScan also need equivalent enhancement?
> >
> > For postgres_fdw, running the query from a parallel worker would
> > change the transaction semantics.  Suppose you begin a transaction,
> > UPDATE data on the foreign server, and then run a parallel query.  If
> > the leader performs the ForeignScan it will see the uncommitted
> > UPDATE, but a worker would have to make its own connection which not
> > be part of the same transaction and which would therefore not see the
> > update.  That's a problem.
> >
> Ah, yes, as long as FDW driver ensure the remote session has no
> uncommitted data, pg_export_snapshot() might provide us an opportunity,
> however, once a session writes something, FDW driver has to prohibit it.
> 
> > Also, for postgres_fdw, and many other FDWs I suspect, the assumption
> > is that most of the work is being done on the remote side, so doing
> > the work in a parallel worker doesn't seem super interesting.  Instead
> > of incurring transfer costs to move the data from remote to local, we
> > incur two sets of transfer costs: first remote to local, then worker
> > to leader.  Ouch.  I think a more promising line of inquiry is to try
> > to provide asynchronous execution when we have something like:
> >
> > Append
> > -> Foreign Scan
> > -> Foreign Scan
> >
> > ...so that we can return a row from whichever Foreign Scan receives
> > data back from the remote server first.
> >
> > So it's not impossible that an FDW author could want this, but mostly
> > probably not.  I think.
> >
> Yes, I also have same opinion. Likely, local parallelism is not
> valuable for the class of FDWs that obtains data from the remote
> server (e.g, postgres_fdw, ...), expect for the case when packing
> and unpacking cost over the network is major bottleneck.
> 
> On the other hands, it will be valuable for the class of FDW that
> performs as a wrapper to local data structure, as like current
> partial seq-scan doing. (e.g, file_fdw, ...)
> Its data source is not under the transaction control, and 'remote
> execution' of these FDWs are eventually executed on the local
> computing resources.
> 
> If I would make a proof-of-concept patch with interface itself, it
> seems to me file_fdw may be a good candidate for this enhancement.
> It is not a field for postgres_fdw.
> 
> Thanks,
> --
> NEC Business Creation Division / PG-Strom Project
> KaiGai Kohei 



pgsql-v9.6-parallel-cspfdw.v1.patch
Description: pgsql-v9.6-parallel-cspfdw.v1.patch

-- 
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] Set search_path + server-prepared statements = cached plan must not change result type

[David G. Johnston]

On Thu, Jan 28, 2016 at 7:48 AM, Vladimir Sitnikov <
sitnikov.vladi...@gmail.com> wrote:

> Robert>Hmm, so in your example, you actually want replanning to be able to
> Robert>change the cached plan's result type?
>
> I want backend to cache _several_ plans behind a single "statement name".
> I want to treat "prepare...exec...deallocate" dance as an optimization
> step for a simple "exec...exec...exec" sequence.
> I do not want to care if "previously prepared query is still valid or
> not". For instance, I do not want to check if search_path is still the
> same.
>
> Current backend implementation does not report changes to
> "search_path", thus clients have no solid way to detect "search_path
> changes".
>
> David>Maybe call the new command "PARSE name AS query".
>
> From JDBC perspective, there is no need in "prepare vs parse" distinction:
> 1) Explicit "prepare...execute" are not used in typical application code
> 2) That means, in 99.9% cases, "prepare" would be used by the jdbc driver
> itself
> 3) Thus just a single "protocol command" is sufficient.
>
> What I am saying is there are lots of consumers that want to avoid
> parsing overhead: plpgsql, pgjdbc, pgjdbc-ng, postgresql-async,
> 8kdata/phoebe, etc, etc.
>
> All of them will have to deal with search_path vs prepare issue.
> If you suggest to deprecate "prepare" in favor of "parse", then all of
> the above clients would have to switch to that "parse".
> It does not look like a good solution, since lots of existing clients
> assume "prepare just works".
>
> If "prepare" command gets deprecated, why "parse" would be better?
> What would be the use of "prepare" if all the clients would have to
> use "parse" in order to be search_path-compatible?
>
>
​Further pondering on this topic reveals that I need a more solid
understanding of the underlying layers...I'm not really sure at this point
whether further redefining the behavior of PREPARE is as undesirable as it
first seemed to be.  It does impose some constraints and makes assumptions
in order to provides its capability and so instead of trying to add yet
more complexity to it in order to fulfill this different use case it can at
least be considered that a different module be provided as a solution.  I
guess if it got to the point where the new facility could supersede PREPARE
you would just modify PREPARE but if they end up performing two different
things then no deprecation would be involved.

David J.

Re: [HACKERS] HEADSUP: gitmaster.postgresql.org - upgrade NOW

[Robert Haas]

On Thu, Jan 28, 2016 at 10:25 AM, Stefan Kaltenbrunner
 wrote:
> On 01/28/2016 04:00 PM, Stefan Kaltenbrunner wrote:
>> Per discussion in the afternoon break at  FOSDEM/PGDay 2016 Developer
>> Meeting we are going to upgrade gemulon.postgresql.org aka "gitmaster"
>> today (the upgrade was originally scheduled end of last year but due to
>> release and other constraints was never executed).
>> The upgrade is going to start in a few minutes - will send a notice
>> after it is done.
>
> upgrade completed - everything should be up again!

That was quick!

Stefan, let me just mention how much I appreciate the work you and the
entire infrastructure team do to keep our project running.  I am less
aware of what all that work is than some people, I know, but I know
that it really makes a difference and certainly I think about how nice
it is to be able to push a commit and know that somebody else has
taken responsibility for making sure it has someplace to which to get
pushed.

So again: thanks.

-- 
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] [WIP] Effective storage of duplicates in B-tree index.

[Peter Geoghegan]

On Thu, Jan 28, 2016 at 9:03 AM, Thom Brown  wrote:
> I'm surprised that efficiencies can't be realised beyond this point.  Your 
> results show a sweet spot at around 1000 / 1000, with it getting slightly 
> worse beyond that.  I kind of expected a lot of efficiency where all the 
> values are the same, but perhaps that's due to my lack of understanding 
> regarding the way they're being stored.

I think that you'd need an I/O bound workload to see significant
benefits. That seems unsurprising. I believe that random I/O from
index writes is a big problem for us.



-- 
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] Using user mapping OID as hash key for connection hash

[Robert Haas]

On Wed, Jan 27, 2016 at 6:32 AM, Ashutosh Bapat
 wrote:
> As discussed in postgres_fdw join pushdown thread [1], for two different
> effective local users which use public user mapping we will be creating two
> different connections to the foreign server with the same credentials.
>
> Robert suggested [2] that we should use user mapping OID as the connection
> cache key instead of current userid and serverid.
>
> There are two patches attached here:
> 1. pg_fdw_concache.patch.short - shorter version of the fix. Right now
> ForeignTable, ForeignServer have corresponding OIDs saved in these
> structures. But UserMapping doesn't. Patch adds user mapping OID as a member
> to this structure. This member is then used as key in GetConnection().
> 2. pg_fdw_concache.patch.large - most of the callers of GetConnection() get
> ForeignServer object just to pass it to GetConnection(). GetConnection can
> obtain ForeignServer by using serverid from UserMapping and doesn't need
> ForeignServer to be an argument. Larger version of patch has this change.

The long form seems like a good idea, so I committed that one.

-- 
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] Template for commit messages

[David Fetter]

On Thu, Jan 28, 2016 at 03:52:25PM +0100, Tom Lane wrote:
> Robert Haas  writes:
> > On Thu, Jan 28, 2016 at 8:04 AM, Tomas Vondra
> >  wrote:
> >> Why can't we do both? That is, have a free-form text with the nuances, and
> >> then Reviewed-By listing the main reviewers? The first one is for humans,
> >> the other one for automated tools.
> 
> > I'm not objecting to or endorsing any specific proposal, just asking
> > what we want to do about this.  I think the trick if we do it that way
> > will be to avoid having it seem like too much duplication, but there
> > may be a way to manage that.
> 
> FWIW, I'm a bit suspicious of the idea that we need to make the commit
> messages automated-tool-friendly.  What tools are there that would need
> to extract this info, and would we trust them if they didn't understand
> "nuances"?
> 
> I'm on board with Bruce's template as being a checklist of points to be
> sure to cover when composing a commit message.  I'm not sure we need
> fixed-format rules.

I've been asking for them for years so I can spend my time on the
PostgreSQL Weekly News more efficiently.  Maybe it's more efficient
for me to do this arranging than for each committer to do it.  I'd
like to imagine that committers are in a better position than I to
summarize their work.

Whatever we decide on here, I'd really appreciate it if every patch
sent to the list came with a sentence describing what that version of
it does, as scope drift frequently makes Subject: lines completely
wrong.

While I'm at it, I'd like to thank Andres Freund, Peter Geoghegan, and
Robert Haas in particular for making a habit of writing detailed
summaries and splitting patches into logical chunks.  All errors in
the PostgreSQL Weekly News are mine, but a little organization like
theirs would go a very long way, and not just for me.

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] Additional role attributes && superuser review

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Sun, Jan 17, 2016 at 6:58 PM, Stephen Frost  wrote:
> > I'm not against that idea, though I continue to feel that there are
> > common sets of privileges which backup tools could leverage.
> >
> > The other issue that I'm running into, again, while considering how to
> > move back to ACL-based permissions for these objects is that we can't
> > grant out the actual permissions which currently exist.  That means we
> > either need to break backwards compatibility, which would be pretty
> > ugly, in my view, or come up with new functions and then users will have
> > to know which functions to use when.
> >
> > As I don't think we really want to break backwards compatibility or
> > remove existing functionality, the only approach which is going to make
> > sense is to add additional functions in some cases.  In particular, we
> > will need alternate versions of pg_terminate_backend and
> > pg_cancel_backend.  One thought I had was to make that
> > 'pg_signal_backend', but that sounds like we'd allow any signal sent by
> > a user with that right, which seems a bit much to me...
> 
> So, this seems like a case where a built-in role would be
> well-justified.  I don't really believe in built-in roles as a way of
> bundling related permissions; I know you do, but I don't.  I'd rather
> see the individual function permissions granted individually.  But
> here you are talking about a variable level of access to the same
> function, depending on role.  And for that it seems to me that a
> built-in role has a lot more to recommend it in that case than it does
> in the other.  If you have been granted pg_whack, you can signal any
> process on the system; otherwise just your own.  Those checks are
> internal to pg_terminate_backend/pg_cancel_backend so GRANT is not a
> substitute.

If we extend this into the future then it seems to potentially fall
afoul of Noah's concern that we're going to end up with two different
ways of saying GRANT EXECUTE X ON Y.  Consider the more complicated case
of pg_stat_activity, where values are shown or hidden based on who the
current role is.  The policy system only supports whole-row, today, but
the question has already come up, both on the lists and off, of support
for hiding individual column values for certain rows, exactly as we do
today for pg_stat_activity.  Once we reach that point, we'll have a way
to GRANT out these rights and a default role which just has them.

Personally, I don't have any particular issue having both, but the
desire was stated that it would be better to have the regular
GRANT EXECUTE ON catalog_func() working before we consider having
default roles for same.  That moves the goal posts awful far though, if
we're to stick with that and consider how we might extend the GRANT
system in the future.

What got me thinking along these lines was a question posed by Bruce
(Bruce, feel free to chime in if I've misunderstood) to me at SCALE
where we were chatting a bit about this, which was- how could we extend
GRANT to support the permissions that we actually wish
pg_terminate_backend() and pg_cancel_backend() to have, instead of using
a default role?  I didn't think too much on it at the time as it strikes
me as a pretty narrow use-case and requiring quite a bit of complexity
to get right, but there again, I'd certainly view a system where the
user is allowed to have pg_start_backup() rights but not
pg_stop_backup() as being quite a small use-case also, yet that's the
direction we're largely going in with this discussion.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Template for commit messages

[Bruce Momjian]

On Thu, Jan 28, 2016 at 10:40:09AM -0500, Stephen Frost wrote:
> * Joshua D. Drake (j...@commandprompt.com) wrote:
> > On 01/28/2016 06:57 AM, Robert Haas wrote:
> > 
> > >>I'm on board with Bruce's template as being a checklist of points to be
> > >>sure to cover when composing a commit message.  I'm not sure we need
> > >>fixed-format rules.
> > >
> > >Well, I think what people are asking for is precisely a fixed format,
> > >and I do think there is value in that.  It's nice to capture the
> > >nuance, but the nuance is going to get flattened out anyway when the
> > >release notes are created.  If we all agree to use a fixed format,
> > >then a bunch of work there that probably has to be done manually can
> > >be automated.  However, if we all agree to use a fixed format except
> > >for you, we might as well just forget the whole thing, because the
> > >percentage of commits that are done by you is quite high.
> > 
> > Yes, we are either all in or we may as well forgo this.
> 
> I don't have a particular issue with it, but would like whatever
> template is decided upon to be included in our git repo and then we
> should be able to make it the template that's opened up when people go
> to commit pretty easily.

OK, but keep in mind whatever script committers user should remove tags
that are empty after exiting the editor.  I can provide the grep regex
in git somewhere too:

  egrep -v "^(Author|Reported-by|Bug|Reviewed-by|Tested-by|Backpatch-through): 
*$"

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


-- 
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] HEADSUP: gitmaster.postgresql.org - upgrade NOW

[Stefan Kaltenbrunner]

On 01/28/2016 05:24 PM, Robert Haas wrote:
> On Thu, Jan 28, 2016 at 10:25 AM, Stefan Kaltenbrunner
>  wrote:
>> On 01/28/2016 04:00 PM, Stefan Kaltenbrunner wrote:
>>> Per discussion in the afternoon break at  FOSDEM/PGDay 2016 Developer
>>> Meeting we are going to upgrade gemulon.postgresql.org aka "gitmaster"
>>> today (the upgrade was originally scheduled end of last year but due to
>>> release and other constraints was never executed).
>>> The upgrade is going to start in a few minutes - will send a notice
>>> after it is done.
>>
>> upgrade completed - everything should be up again!
> 
> That was quick!

if my math is correct that was the 49th machine (with some more to come)
upgraded from wheezy to jessie - I think we have some routine now :)

> 
> Stefan, let me just mention how much I appreciate the work you and the
> entire infrastructure team do to keep our project running.  I am less
> aware of what all that work is than some people, I know, but I know
> that it really makes a difference and certainly I think about how nice
> it is to be able to push a commit and know that somebody else has
> taken responsibility for making sure it has someplace to which to get
> pushed.
> 
> So again: thanks.

on behalf of the entire team - thanks for the appreciation, we are doing
our best not get noticed (because in that case something would be
broken) :)


Stefan


-- 
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] CustomScan under the Gather node?

[Robert Haas]

On Thu, Jan 28, 2016 at 10:50 AM, Kouhei Kaigai  wrote:
>> If I would make a proof-of-concept patch with interface itself, it
>> seems to me file_fdw may be a good candidate for this enhancement.
>> It is not a field for postgres_fdw.
>>
> The attached patch is enhancement of FDW/CSP interface and PoC feature
> of file_fdw to scan source file partially. It was smaller enhancement
> than my expectations.
>
> It works as follows. This query tried to read 20M rows from a CSV file,
> using 3 background worker processes.
>
> postgres=# set max_parallel_degree = 3;
> SET
> postgres=# explain analyze select * from test_csv where id % 20 = 6;
>   QUERY PLAN
> 
>  Gather  (cost=1000.00..194108.60 rows=94056 width=52)
>  (actual time=0.570..19268.010 rows=200 loops=1)
>Number of Workers: 3
>->  Parallel Foreign Scan on test_csv  (cost=0.00..183703.00 rows=94056 
> width=52)
>   (actual time=0.180..12744.655 rows=50 
> loops=4)
>  Filter: ((id % 20) = 6)
>  Rows Removed by Filter: 950
>  Foreign File: /tmp/testdata.csv
>  Foreign File Size: 1504892535
>  Planning time: 0.147 ms
>  Execution time: 19330.201 ms
> (9 rows)

Could you try it not in parallel and then with 1, 2, 3, and 4 workers
and post the times for all?

-- 
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] extend pgbench expressions with functions

[Fabien COELHO]

So I'm arguing that exiting, with an error message, is better than handling
user errors.


I'm not objecting to exiting with an error message, but I think
letting ourselves be killed by a signal is no good.


Ok, I understand this point for this purpose.

--
Fabien.


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

Re: [HACKERS] [WIP] Effective storage of duplicates in B-tree index.

[Anastasia Lubennikova]

28.01.2016 18:12, Thom Brown:
On 28 January 2016 at 14:06, Anastasia Lubennikova 
> 
wrote:



31.08.2015 10:41, Anastasia Lubennikova:

Hi, hackers!
I'm going to begin work on effective storage of duplicate keys in
B-tree index.
The main idea is to implement posting lists and posting trees for
B-tree index pages as it's already done for GIN.

In a nutshell, effective storing of duplicates in GIN is
organised as follows.
Index stores single index tuple for each unique key. That index
tuple points to posting list which contains pointers to heap
tuples (TIDs). If too many rows having the same key, multiple
pages are allocated for the TIDs and these constitute so called
posting tree.
You can find wonderful detailed descriptions in gin readme


and articles .
It also makes possible to apply compression algorithm to posting
list/tree and significantly decrease index size. Read more in
presentation (part 1)
.

Now new B-tree index tuple must be inserted for each table row
that we index.
It can possibly cause page split. Because of MVCC even unique
index could contain duplicates.
Storing duplicates in posting list/tree helps to avoid
superfluous splits.


I'd like to share the progress of my work. So here is a WIP patch.
It provides effective duplicate handling using posting lists the
same way as GIN does it.

Layout of the tuples on the page is changed in the following way:
before:
TID (ip_blkid, ip_posid) + key, TID (ip_blkid, ip_posid) + key,
TID (ip_blkid, ip_posid) + key
with patch:
TID (N item pointers, posting list offset) + key, TID (ip_blkid,
ip_posid), TID (ip_blkid, ip_posid), TID (ip_blkid, ip_posid)

It seems that backward compatibility works well without any
changes. But I haven't tested it properly yet.

Here are some test results. They are obtained by test functions
test_btbuild and test_ginbuild, which you can find in attached sql
file.
i - number of distinct values in the index. So i=1 means that all
rows have the same key, and i=1000 means that all keys are
different.
The other columns contain the index size (MB).

i   B-tree Old  B-tree New  GIN
1   214,234375  87,7109375  10,2109375
10  214,234375  87,7109375  10,71875
100 214,234375  87,4375 15,640625
1000214,234375  86,2578125  31,296875
1   214,234375  78,421875   104,3046875
10  214,234375  65,359375   49,078125
100 214,234375  90,140625   106,8203125
1000214,234375  214,234375  534,0625


You can note that the last row contains the same index sizes for
B-tree, which is quite logical - there is no compression if all
the keys are distinct.
Other cases looks really nice to me.
Next thing to say is that I haven't implemented posting list
compression yet. So there is still potential to decrease size of
compressed btree.

I'm almost sure, there are still some tiny bugs and missed
functions, but on the whole, the patch is ready for testing.
I'd like to get a feedback about the patch testing on some real
datasets. Any bug reports and suggestions are welcome.

Here is a couple of useful queries to inspect the data inside the
index pages:
create extension pageinspect;
select * from bt_metap('idx');
select bt.* from generate_series(1,1) as n, lateral
bt_page_stats('idx', n) as bt;
select n, bt.* from generate_series(1,1) as n, lateral
bt_page_items('idx', n) as bt;

And at last, the list of items I'm going to complete in the near
future:
1. Add storage_parameter 'enable_compression' for btree access
method which specifies whether the index handles duplicates.
default is 'off'
2. Bring back microvacuum functionality for compressed indexes.
3. Improve insertion speed. Insertions became significantly slower
with compressed btree, which is obviously not what we do want.
4. Clean the code and comments, add related documentation.


This doesn't apply cleanly against current git head. Have you caught 
up past commit 65c5fcd35?


Thank you for the notice. New patch is attached.

--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 9673fe0..0c8e4fb 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -495,7 +495,7 @@ 

Re: [HACKERS] tiny doc patch

[Robert Haas]

On Thu, Jan 28, 2016 at 9:09 AM, Filip Rembiałkowski
 wrote:
> right.

Committed and back-patched to 9.5.

-- 
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] New committer

[Peter Geoghegan]

On Thu, Jan 28, 2016 at 6:37 AM, Magnus Hagander  wrote:
> The PostgreSQL core team would like to welcome Dean Rasheed as a new
> committer for the PostgreSQL project.

Congratulations, Dean. Well deserved.


-- 
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] Template for commit messages

[Joshua D. Drake]

On 01/28/2016 06:34 AM, Bruce Momjian wrote:

On Thu, Jan 28, 2016 at 09:31:46AM -0500, Robert Haas wrote:

On Thu, Jan 28, 2016 at 6:16 AM, Tomas Vondra
 wrote:

How about
User-Interface-Bikeshedded-By:
?


+1


That's sort of implicitly pejorative.   Maybe we could find another
way to phrase that, like "Design-suggestions-by" or maybe a more
general "Suggestions-by".


I wasn't sure if "User-Interface-Bikeshedded-By" related to the patch,
or to the discussion of labels for the commit messages.


Contribution-by:

Definition: Individuals who provided notable review which may or may not 
have been code review.


Sincerely,

Joshua D. Drake


--
Command Prompt, Inc.  http://the.postgres.company/
 +1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.


--
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] insufficient qualification of some objects in dump files

[Michael Paquier]

On Fri, Jan 29, 2016 at 1:17 AM, Peter Eisentraut  wrote:
> Some dump objects whose names are not unique on a schema level have
> insufficient details in the dump TOC.  For example, a column default
> might have a TOC entry like this:
>
> 2153; 2604 39696 DEFAULT public a rolename
>
> Note that this only contains the schema name and the column name, but
> not the table name.  So this makes it impossible to filter the TOC by
> text search in situations like this.
>
> It looks like other object types such as triggers have the same problem.
>
> I think we should amend the archive tag for these kinds of objects to
> include the table name, so it might look like:
>
> 2153; 2604 39696 DEFAULT public test a rolename
>
> Comments?

+1. I noticed that this limitation is present for triggers (as you
mentioned), constraints, fk constraints and RLS policies which should
be completed with a table name.
-- 
Michael


-- 
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] extend pgbench expressions with functions

[Fabien COELHO]

Hello Michaël,

v23 attached, which does not change the message but does the other fixes.


+if (coerceToInt() == INT64_MIN && coerceToInt() == -1)
+{
+   fprintf(stderr, "cannot divide INT64_MIN by -1\n");
+   return false;
+}
Bike-shedding: "bigint out of range" to match what is done in the backend?


ISTM that it is clearer for the user to say that the issue is with the 
division? Otherwise the message does not help much. Well, not that it 
would be printed often...



+/*
+ * Recursive evaluation of an expression in a pgbench script
+ * using the current state of variables.
+ * Returns whether the evaluation was ok,
+ * the value itself is returned through the retval pointer.
+ */
Could you reformat this comment?


I can.


fprintf(stderr,
-"exponential parameter must be
greater than zero (not \"%s\")\n",
-argv[5]);
+ "exponential parameter must be greater than
zero (not \"%s\")\n",
+  argv[5]);
This is some unnecessary diff noise.


Indeed.


+setIntValue(retval, getGaussianRand(thread, arg1, arg2,
 dparam));
Some portions of the code are using tabs instead of spaces between
function arguments.


Indeed.

I would as well suggest fixing first the (INT64_MAX / -1) crash on HEAD 
and back-branches with something like the patch attached, inspired from 
int8.c.


I think it is overkill, but do as you feel.

Note that it must also handle modulo, but the code you suggest cannot be 
used for that.


  #include 
  int main(int argc, char* argv[])
  {
int64_t l = INT64_MIN;
int64_t r = -1;
int64_t d = l % r;
return 0;
  }
  // => Floating point exception (core dumped)

ISTM that the second condition can be simplified, as there is no possible 
issue if lval is positive?


   if (lval < 0 && *resval < 0) { ... }

--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 42d0667..d42208a 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -796,17 +796,21 @@ pgbench  options  dbname
   Sets variable varname to an integer value calculated
   from expression.
   The expression may contain integer constants such as 5432,
-  references to variables :variablename,
+  double constants such as 3.14159,
+  references to integer variables :variablename,
   and expressions composed of unary (-) or binary operators
-  (+, -, *, /, %)
-  with their usual associativity, and parentheses.
+  (+, -, *, /,
+  %) with their usual associativity, function calls and
+  parentheses.
+   shows the available
+  functions.
  
 
  
   Examples:
 
 \set ntellers 10 * :scale
-\set aid (1021 * :aid) % (10 * :scale) + 1
+\set aid (1021 * random(1, 10 * :scale)) % (10 * :scale) + 1
 
 

@@ -826,66 +830,35 @@ pgbench  options  dbname
  
 
  
-  By default, or when uniform is specified, all values in the
-  range are drawn with equal probability.  Specifying gaussian
-  or  exponential options modifies this behavior; each
-  requires a mandatory parameter which determines the precise shape of the
-  distribution.
- 
+  
+   
+
+ \setrandom n 1 10 or \setrandom n 1 10 uniform
+ is equivalent to \set n random(1, 10) and uses a uniform
+ distribution.
+
+   
 
- 
-  For a Gaussian distribution, the interval is mapped onto a standard
-  normal distribution (the classical bell-shaped Gaussian curve) truncated
-  at -parameter on the left and +parameter
-  on the right.
-  Values in the middle of the interval are more likely to be drawn.
-  To be precise, if PHI(x) is the cumulative distribution
-  function of the standard normal distribution, with mean mu
-  defined as (max + min) / 2.0, with
-
- f(x) = PHI(2.0 * parameter * (x - mu) / (max - min + 1)) /
-(2.0 * PHI(parameter) - 1.0)
-
-  then value i between min and
-  max inclusive is drawn with probability:
-  f(i + 0.5) - f(i - 0.5).
-  Intuitively, the larger parameter, the more
-  frequently values close to the middle of the interval are drawn, and the
-  less frequently values close to the min and
-  max bounds. About 67% of values are drawn from the
-  middle 1.0 / parameter, that is a relative
-  0.5 / parameter around the mean, and 95% in the middle
-  2.0 / parameter, that is a relative
-  1.0 / parameter around the mean; for instance, if
-  parameter is 4.0, 67% of values are drawn from the
-  middle quarter (1.0 / 4.0) of the interval (i.e. from
-  3.0 / 8.0 to 5.0 / 8.0) and 95% from
-  the middle half (2.0 / 4.0) of the interval (second and
-  third quartiles). The minimum parameter is 2.0 for
-  performance of the Box-Muller transform.
-   

Re: [HACKERS] extend pgbench expressions with functions

[Michael Paquier]

On Fri, Jan 29, 2016 at 3:40 PM, Fabien COELHO  wrote:

>
> I would as well suggest fixing first the (INT64_MAX / -1) crash on HEAD
>> and back-branches with something like the patch attached, inspired from
>> int8.c.
>>
>
> I think it is overkill, but do as you feel.
>

Perhaps we could have Robert decide on this one first? That's a bug after
all that had better be backpatched.


> Note that it must also handle modulo, but the code you suggest cannot be
> used for that.
>
>   #include 
>   int main(int argc, char* argv[])
>   {
> int64_t l = INT64_MIN;
> int64_t r = -1;
> int64_t d = l % r;
> return 0;
>   }
>   // => Floating point exception (core dumped)
>

Right, forgot this one, we just need to check if rval is -1 here, and
return 0 as result. I am updating the fix as attached.
-- 
Michael
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index d5f242c..25b349d 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -967,7 +967,28 @@ evaluateExpr(CState *st, PgBenchExpr *expr, int64 *retval)
 			fprintf(stderr, "division by zero\n");
 			return false;
 		}
-		*retval = lval / rval;
+		/*
+		 * INT64_MIN / -1 is problematic, since the result
+		 * can't be represented on a two's-complement
+		 * machine. Some machines produce INT64_MIN, some
+		 * produce zero, some throw an exception. We can
+		 * dodge the problem by recognizing that division
+		 * by -1 is the same as negation.
+		 */
+		if (rval == -1)
+		{
+			*retval = -lval;
+
+			/* overflow check (needed for INT64_MIN) */
+			if (lval != 0 && (*retval < 0 == lval < 0))
+			{
+fprintf(stderr, "bigint out of range\n");
+return false;
+			}
+		}
+		else
+			*retval = lval / rval;
+
 		return true;
 
 	case '%':
@@ -976,7 +997,15 @@ evaluateExpr(CState *st, PgBenchExpr *expr, int64 *retval)
 			fprintf(stderr, "division by zero\n");
 			return false;
 		}
-		*retval = lval % rval;
+		/*
+		 * Some machines throw a floating-point exception
+		 * for INT64_MIN % -1, the correct answer being
+		 * zero in any case.
+		 */
+		if (rval == -1)
+			*retval = 0;
+		else
+			*retval = lval % rval;
 		return true;
 }
 

-- 
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] insufficient qualification of some objects in dump files

[Michael Paquier]

On Fri, Jan 29, 2016 at 3:51 PM, Tom Lane  wrote:
>
> Michael Paquier  writes:
> > On Fri, Jan 29, 2016 at 1:17 AM, Peter Eisentraut  wrote:
> >> I think we should amend the archive tag for these kinds of objects to
> >> include the table name, so it might look like:
> >>
> >> 2153; 2604 39696 DEFAULT public test a rolename
>
> > +1. I noticed that this limitation is present for triggers (as you
> > mentioned), constraints, fk constraints and RLS policies which should
> > be completed with a table name.
>
> How can we do this without an archive format version bump ... or were
> you assuming that that would be an acceptable price?  (It's not like
> we haven't done those before, so maybe it is.)

Yes, I am assuming that's worth the price, many people run similar
relation schemas on the same database with different schema names. And
Peter has a point that the current format can be confusing for the
user. Sorry if I sounded like it was a bug that should be backpatched
or something similar, I don't mean that.
-- 
Michael


-- 
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] insufficient qualification of some objects in dump files

[Tom Lane]

Michael Paquier  writes:
> On Fri, Jan 29, 2016 at 1:17 AM, Peter Eisentraut  wrote:
>> I think we should amend the archive tag for these kinds of objects to
>> include the table name, so it might look like:
>> 
>> 2153; 2604 39696 DEFAULT public test a rolename

> +1. I noticed that this limitation is present for triggers (as you
> mentioned), constraints, fk constraints and RLS policies which should
> be completed with a table name.

How can we do this without an archive format version bump ... or were
you assuming that that would be an acceptable price?  (It's not like
we haven't done those before, so maybe it is.)

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] HEADSUP: gitmaster.postgresql.org - upgrade NOW

[Stefan Kaltenbrunner]

On 01/28/2016 04:00 PM, Stefan Kaltenbrunner wrote:
> Hi all!
> 
> Per discussion in the afternoon break at  FOSDEM/PGDay 2016 Developer
> Meeting we are going to upgrade gemulon.postgresql.org aka "gitmaster"
> today (the upgrade was originally scheduled end of last year but due to
> release and other constraints was never executed).
> The upgrade is going to start in a few minutes - will send a notice
> after it is done.


upgrade completed - everything should be up again!



Stefan


-- 
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] Fwd: Core dump with nested CREATE TEMP TABLE

[Robert Haas]

On Wed, Jan 27, 2016 at 11:47 PM, Noah Misch  wrote:
> On Wed, Jan 27, 2016 at 11:04:33PM -0500, Robert Haas wrote:
>> +Assert(portal->status != PORTAL_ACTIVE);
>>  if (portal->status == PORTAL_ACTIVE)
>>  MarkPortalFailed(portal);
>>
>> Now that just looks kooky to me.  We assert that the portal isn't
>> active, but then cater to the possibility that it might be anyway?
>
> Right.
>
>> That seems totally contrary to our usual programming practices, and a
>> bad idea for that reason.
>
> It is contrary to our usual programming practices, I agree.  I borrowed the
> idea from untenured code (da3751c8, 2015-11-11) in load_relcache_init_file():
>
> if (nailed_rels != NUM_CRITICAL_SHARED_RELS ||
> nailed_indexes != NUM_CRITICAL_SHARED_INDEXES)
> {
> elog(WARNING, "found %d nailed shared rels and %d 
> nailed shared indexes in init file, but expected %d and %d respectively",
>  nailed_rels, nailed_indexes,
>  NUM_CRITICAL_SHARED_RELS, 
> NUM_CRITICAL_SHARED_INDEXES);
> /* Make sure we get developers' attention about this 
> */
> Assert(false);
>
> I liked this pattern.  It's a good fit for cases that we design to be
> impossible yet for which we have a workaround if they do happen.  That being
> said, if you feel it's bad, I would be fine using elog(FATAL).  I envision
> this as a master-only change in any case.  No PGXN module references
> PORTAL_ACTIVE or MarkPortalActive(), so it's unlikely that extension code will
> notice the change whether in Assert() form or in elog() form.  What is best?

I'm honestly failing to understand why we should change anything at
all.  I don't believe that doing something more severe than marking
the portal failed actually improves anything.  I suppose if I had to
pick between what you proposed before and elog(FATAL) I'd pick the
latter, but I see no real reason to cut off future code (or
third-party code) at the knees like that.  I don't see it as either
necessary or desirable to forbid something just because there's no
clear present use case for it.  The code you quote emits a warning
about a reasonably forseeable situation that can never be right, but
there's no particular reason to think that MarkPortalFailed is the
wrong thing to do here if that situation came up.

-- 
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] Additional role attributes && superuser review

[Robert Haas]

On Thu, Jan 28, 2016 at 11:04 AM, Stephen Frost  wrote:
>> So, this seems like a case where a built-in role would be
>> well-justified.  I don't really believe in built-in roles as a way of
>> bundling related permissions; I know you do, but I don't.  I'd rather
>> see the individual function permissions granted individually.  But
>> here you are talking about a variable level of access to the same
>> function, depending on role.  And for that it seems to me that a
>> built-in role has a lot more to recommend it in that case than it does
>> in the other.  If you have been granted pg_whack, you can signal any
>> process on the system; otherwise just your own.  Those checks are
>> internal to pg_terminate_backend/pg_cancel_backend so GRANT is not a
>> substitute.
>
> If we extend this into the future then it seems to potentially fall
> afoul of Noah's concern that we're going to end up with two different
> ways of saying GRANT EXECUTE X ON Y.  Consider the more complicated case
> of pg_stat_activity, where values are shown or hidden based on who the
> current role is.  The policy system only supports whole-row, today, but
> the question has already come up, both on the lists and off, of support
> for hiding individual column values for certain rows, exactly as we do
> today for pg_stat_activity.  Once we reach that point, we'll have a way
> to GRANT out these rights and a default role which just has them.

Well, I'm not saying that predefined rolls are the *only* way to solve
a problem like this, but I think they're one way and I don't clearly
see that something else is better.  However, my precise point is that
we should *not* have predefined rolls that precisely duplicate the
result of GRANT EXECUTE X ON Y.  Having predefined rolls that change
the behavior of the system in other ways is a different thing.  So I
don't see how this falls afoul of Noah's concern.  (If it does,
perhaps he can clarify.)

> Personally, I don't have any particular issue having both, but the
> desire was stated that it would be better to have the regular
> GRANT EXECUTE ON catalog_func() working before we consider having
> default roles for same.  That moves the goal posts awful far though, if
> we're to stick with that and consider how we might extend the GRANT
> system in the future.

I don't think it moves the goal posts all that far.  Convincing
pg_dump to dump grants on system functions shouldn't be a crazy large
patch.

> What got me thinking along these lines was a question posed by Bruce
> (Bruce, feel free to chime in if I've misunderstood) to me at SCALE
> where we were chatting a bit about this, which was- how could we extend
> GRANT to support the permissions that we actually wish
> pg_terminate_backend() and pg_cancel_backend() to have, instead of using
> a default role?  I didn't think too much on it at the time as it strikes
> me as a pretty narrow use-case and requiring quite a bit of complexity
> to get right, but there again, I'd certainly view a system where the
> user is allowed to have pg_start_backup() rights but not
> pg_stop_backup() as being quite a small use-case also, yet that's the
> direction we're largely going in with this discussion.

Well, sure, in that largely artificial example it's not hard to say
ha, ha, silly, but the actual examples we looked at upthread were much
less obviously silly.  There was plenty of room for argument about the
precise contours of each predefined role.

-- 
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] [WIP] Effective storage of duplicates in B-tree index.

[Thom Brown]

On 28 January 2016 at 17:09, Peter Geoghegan  wrote:
> On Thu, Jan 28, 2016 at 9:03 AM, Thom Brown  wrote:
>> I'm surprised that efficiencies can't be realised beyond this point.  Your 
>> results show a sweet spot at around 1000 / 1000, with it getting 
>> slightly worse beyond that.  I kind of expected a lot of efficiency where 
>> all the values are the same, but perhaps that's due to my lack of 
>> understanding regarding the way they're being stored.
>
> I think that you'd need an I/O bound workload to see significant
> benefits. That seems unsurprising. I believe that random I/O from
> index writes is a big problem for us.

I was thinking more from the point of view of the index size.  An
index containing 10 million duplicate values is around 40% of the size
of an index with 10 million unique values.

Thom


-- 
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] Template for commit messages

[Tomas Vondra]

On 01/28/2016 03:37 PM, Robert Haas wrote:

On Thu, Jan 28, 2016 at 9:34 AM, Bruce Momjian  wrote:

On Thu, Jan 28, 2016 at 09:31:46AM -0500, Robert Haas wrote:

On Thu, Jan 28, 2016 at 6:16 AM, Tomas Vondra
 wrote:

How about
User-Interface-Bikeshedded-By:
?


+1


That's sort of implicitly pejorative.   Maybe we could find another
way to phrase that, like "Design-suggestions-by" or maybe a more
general "Suggestions-by".


I wasn't sure if "User-Interface-Bikeshedded-By" related to the patch,
or to the discussion of labels for the commit messages.


I thought it was a proposed commit message label and that the original
suggestion was tongue-in-cheek, but I might be misreading things.


Perhaps I'm wrong, but I believe that particular header was not really 
meant seriously.



--
Tomas Vondra  http://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:Use PGDLLEXPORT for libpq

[Yury Zhuravlev]

Michael Paquier wrote:

Well, 2015 is not making things easy visibly by not providing direct
ways to get locale information.

pgwin32_putenv broken too...
--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres 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] postgres_fdw join pushdown (was Re: Custom/Foreign-Join-APIs)

[Robert Haas]

On Thu, Jan 28, 2016 at 11:26 AM, Ashutosh Bapat
 wrote:
> 1. pg_fdw_core_v3.patch: changes in core - more description below

I've committed most of this patch, with some modifications.  In
particular, I moved CachedPlanSource's hasForeignJoin flag to the
CachedPlan and renamed it has_foreign_join, consistent with the naming
of other members.

The GetUserMappingById() function seemed like a separate thing, so I
left that out of this commit.

-- 
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] extend pgbench expressions with functions

[Fabien COELHO]

v22 compared to previous:
 - remove the short macros (although IMO it is a code degradation)
 - try not to remove/add blanks lines
 - let some assert "as is"
 - still exit on float to int overflow, see arguments in other mails
 - check for INT64_MIN / -1 (although I think it is useless)

--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 42d0667..d42208a 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -796,17 +796,21 @@ pgbench  options  dbname
   Sets variable varname to an integer value calculated
   from expression.
   The expression may contain integer constants such as 5432,
-  references to variables :variablename,
+  double constants such as 3.14159,
+  references to integer variables :variablename,
   and expressions composed of unary (-) or binary operators
-  (+, -, *, /, %)
-  with their usual associativity, and parentheses.
+  (+, -, *, /,
+  %) with their usual associativity, function calls and
+  parentheses.
+   shows the available
+  functions.
  
 
  
   Examples:
 
 \set ntellers 10 * :scale
-\set aid (1021 * :aid) % (10 * :scale) + 1
+\set aid (1021 * random(1, 10 * :scale)) % (10 * :scale) + 1
 
 

@@ -826,66 +830,35 @@ pgbench  options  dbname
  
 
  
-  By default, or when uniform is specified, all values in the
-  range are drawn with equal probability.  Specifying gaussian
-  or  exponential options modifies this behavior; each
-  requires a mandatory parameter which determines the precise shape of the
-  distribution.
- 
+  
+   
+
+ \setrandom n 1 10 or \setrandom n 1 10 uniform
+ is equivalent to \set n random(1, 10) and uses a uniform
+ distribution.
+
+   
 
- 
-  For a Gaussian distribution, the interval is mapped onto a standard
-  normal distribution (the classical bell-shaped Gaussian curve) truncated
-  at -parameter on the left and +parameter
-  on the right.
-  Values in the middle of the interval are more likely to be drawn.
-  To be precise, if PHI(x) is the cumulative distribution
-  function of the standard normal distribution, with mean mu
-  defined as (max + min) / 2.0, with
-
- f(x) = PHI(2.0 * parameter * (x - mu) / (max - min + 1)) /
-(2.0 * PHI(parameter) - 1.0)
-
-  then value i between min and
-  max inclusive is drawn with probability:
-  f(i + 0.5) - f(i - 0.5).
-  Intuitively, the larger parameter, the more
-  frequently values close to the middle of the interval are drawn, and the
-  less frequently values close to the min and
-  max bounds. About 67% of values are drawn from the
-  middle 1.0 / parameter, that is a relative
-  0.5 / parameter around the mean, and 95% in the middle
-  2.0 / parameter, that is a relative
-  1.0 / parameter around the mean; for instance, if
-  parameter is 4.0, 67% of values are drawn from the
-  middle quarter (1.0 / 4.0) of the interval (i.e. from
-  3.0 / 8.0 to 5.0 / 8.0) and 95% from
-  the middle half (2.0 / 4.0) of the interval (second and
-  third quartiles). The minimum parameter is 2.0 for
-  performance of the Box-Muller transform.
- 
+  
+   
+\setrandom n 1 10 exponential 3.0 is equivalent to
+\set n random_exponential(1, 10, 3.0) and uses an
+exponential distribution.
+   
+  
 
- 
-  For an exponential distribution, parameter
-  controls the distribution by truncating a quickly-decreasing
-  exponential distribution at parameter, and then
-  projecting onto integers between the bounds.
-  To be precise, with
-
-f(x) = exp(-parameter * (x - min) / (max - min + 1)) / (1.0 - exp(-parameter))
-
-  Then value i between min and
-  max inclusive is drawn with probability:
-  f(x) - f(x + 1).
-  Intuitively, the larger parameter, the more
-  frequently values close to min are accessed, and the
-  less frequently values close to max are accessed.
-  The closer to 0 parameter, the flatter (more uniform)
-  the access distribution.
-  A crude approximation of the distribution is that the most frequent 1%
-  values in the range, close to min, are drawn
-  parameter% of the time.
-  parameter value must be strictly positive.
+  
+   
+\setrandom n 1 10 gaussian 2.0 is equivalent to
+\set n random_gaussian(1, 10, 2.0), and uses a gaussian
+distribution.
+   
+  
+ 
+
+   See the documentation of these functions below for further information
+   about the precise shape of these distributions, depending on the value
+   of the parameter.
  
 
  
@@ -965,18 +938,184 @@ f(x) = exp(-parameter * (x - min) / (max - min + 1)) / (1.0 - 

Re: [HACKERS] Template for commit messages

[Stephen Frost]

* Bruce Momjian (br...@momjian.us) wrote:
> On Thu, Jan 28, 2016 at 10:40:09AM -0500, Stephen Frost wrote:
> > * Joshua D. Drake (j...@commandprompt.com) wrote:
> > > On 01/28/2016 06:57 AM, Robert Haas wrote:
> > > 
> > > >>I'm on board with Bruce's template as being a checklist of points to be
> > > >>sure to cover when composing a commit message.  I'm not sure we need
> > > >>fixed-format rules.
> > > >
> > > >Well, I think what people are asking for is precisely a fixed format,
> > > >and I do think there is value in that.  It's nice to capture the
> > > >nuance, but the nuance is going to get flattened out anyway when the
> > > >release notes are created.  If we all agree to use a fixed format,
> > > >then a bunch of work there that probably has to be done manually can
> > > >be automated.  However, if we all agree to use a fixed format except
> > > >for you, we might as well just forget the whole thing, because the
> > > >percentage of commits that are done by you is quite high.
> > > 
> > > Yes, we are either all in or we may as well forgo this.
> > 
> > I don't have a particular issue with it, but would like whatever
> > template is decided upon to be included in our git repo and then we
> > should be able to make it the template that's opened up when people go
> > to commit pretty easily.
> 
> OK, but keep in mind whatever script committers user should remove tags
> that are empty after exiting the editor.  I can provide the grep regex
> in git somewhere too:
> 
>   egrep -v 
> "^(Author|Reported-by|Bug|Reviewed-by|Tested-by|Backpatch-through): *$"

If the template is there then, for my part at least, I wouldn't mind
killing the empty lines.  Having a decent script would work too, of
course... but I have to admit that I've not tried having a script modify
my commit messages right before they're committed and, well, it'd take a
bit for me to be comfortable with it.

No one wants garbled commit messages from a script that went awry. ;)

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [WIP] Effective storage of duplicates in B-tree index.

[Thom Brown]

On 28 January 2016 at 17:03, Thom Brown  wrote:

>
> On 28 January 2016 at 16:12, Anastasia Lubennikova <
> a.lubennik...@postgrespro.ru> wrote:
>
>>
>> 28.01.2016 18:12, Thom Brown:
>>
>> On 28 January 2016 at 14:06, Anastasia Lubennikova <
>> a.lubennik...@postgrespro.ru> wrote:
>>
>>>
>>> 31.08.2015 10:41, Anastasia Lubennikova:
>>>
>>> Hi, hackers!
>>> I'm going to begin work on effective storage of duplicate keys in B-tree
>>> index.
>>> The main idea is to implement posting lists and posting trees for B-tree
>>> index pages as it's already done for GIN.
>>>
>>> In a nutshell, effective storing of duplicates in GIN is organised as
>>> follows.
>>> Index stores single index tuple for each unique key. That index tuple
>>> points to posting list which contains pointers to heap tuples (TIDs). If
>>> too many rows having the same key, multiple pages are allocated for the
>>> TIDs and these constitute so called posting tree.
>>> You can find wonderful detailed descriptions in gin readme
>>> 
>>> and articles .
>>> It also makes possible to apply compression algorithm to posting
>>> list/tree and significantly decrease index size. Read more in presentation
>>> (part 1)
>>> .
>>>
>>> Now new B-tree index tuple must be inserted for each table row that we
>>> index.
>>> It can possibly cause page split. Because of MVCC even unique index
>>> could contain duplicates.
>>> Storing duplicates in posting list/tree helps to avoid superfluous
>>> splits.
>>>
>>>
>>> I'd like to share the progress of my work. So here is a WIP patch.
>>> It provides effective duplicate handling using posting lists the same
>>> way as GIN does it.
>>>
>>> Layout of the tuples on the page is changed in the following way:
>>> before:
>>> TID (ip_blkid, ip_posid) + key, TID (ip_blkid, ip_posid) + key, TID
>>> (ip_blkid, ip_posid) + key
>>> with patch:
>>> TID (N item pointers, posting list offset) + key, TID (ip_blkid,
>>> ip_posid), TID (ip_blkid, ip_posid), TID (ip_blkid, ip_posid)
>>>
>>> It seems that backward compatibility works well without any changes. But
>>> I haven't tested it properly yet.
>>>
>>> Here are some test results. They are obtained by test functions
>>> test_btbuild and test_ginbuild, which you can find in attached sql file.
>>> i - number of distinct values in the index. So i=1 means that all rows
>>> have the same key, and i=1000 means that all keys are different.
>>> The other columns contain the index size (MB).
>>>
>>> i B-tree Old B-tree New GIN
>>> 1 214,234375 87,7109375 10,2109375
>>> 10 214,234375 87,7109375 10,71875
>>> 100 214,234375 87,4375 15,640625
>>> 1000 214,234375 86,2578125 31,296875
>>> 1 214,234375 78,421875 104,3046875
>>> 10 214,234375 65,359375 49,078125
>>> 100 214,234375 90,140625 106,8203125
>>> 1000 214,234375 214,234375 534,0625
>>> You can note that the last row contains the same index sizes for B-tree,
>>> which is quite logical - there is no compression if all the keys are
>>> distinct.
>>> Other cases looks really nice to me.
>>> Next thing to say is that I haven't implemented posting list compression
>>> yet. So there is still potential to decrease size of compressed btree.
>>>
>>> I'm almost sure, there are still some tiny bugs and missed functions,
>>> but on the whole, the patch is ready for testing.
>>> I'd like to get a feedback about the patch testing on some real
>>> datasets. Any bug reports and suggestions are welcome.
>>>
>>> Here is a couple of useful queries to inspect the data inside the index
>>> pages:
>>> create extension pageinspect;
>>> select * from bt_metap('idx');
>>> select bt.* from generate_series(1,1) as n, lateral bt_page_stats('idx',
>>> n) as bt;
>>> select n, bt.* from generate_series(1,1) as n, lateral
>>> bt_page_items('idx', n) as bt;
>>>
>>> And at last, the list of items I'm going to complete in the near future:
>>> 1. Add storage_parameter 'enable_compression' for btree access method
>>> which specifies whether the index handles duplicates. default is 'off'
>>> 2. Bring back microvacuum functionality for compressed indexes.
>>> 3. Improve insertion speed. Insertions became significantly slower with
>>> compressed btree, which is obviously not what we do want.
>>> 4. Clean the code and comments, add related documentation.
>>>
>>
>> This doesn't apply cleanly against current git head.  Have you caught up
>> past commit 65c5fcd35?
>>
>>
>> Thank you for the notice. New patch is attached.
>>
>
> Thanks for the quick rebase.
>
> Okay, a quick check with pgbench:
>
> CREATE INDEX ON pgbench_accounts(bid);
>
> Timing
> Scale: master / patch
> 100: 10657ms / 13555ms (rechecked and got 9745ms)
> 500: 56909ms / 56985ms
>
> Size
> Scale: master / patch
> 100: 214MB / 87MB (40.7%)
> 500: 1071MB / 437MB (40.8%)
>
> No performance 

Re: [HACKERS] thanks for FOSDEM/PGDay 2016 Developer Meeting

[Amit Langote]

On Friday, 29 January 2016, Oleg Bartunov  wrote:

> I read
> https://wiki.postgresql.org/wiki/FOSDEM/PGDay_2016_Developer_Meeting and
> would like to say thanks for such nice review of meeting.
>

+many

Thanks,
Amit

Re: [HACKERS] Additional role attributes && superuser review

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Thu, Jan 28, 2016 at 11:04 AM, Stephen Frost  wrote:
> >> So, this seems like a case where a built-in role would be
> >> well-justified.  I don't really believe in built-in roles as a way of
> >> bundling related permissions; I know you do, but I don't.  I'd rather
> >> see the individual function permissions granted individually.  But
> >> here you are talking about a variable level of access to the same
> >> function, depending on role.  And for that it seems to me that a
> >> built-in role has a lot more to recommend it in that case than it does
> >> in the other.  If you have been granted pg_whack, you can signal any
> >> process on the system; otherwise just your own.  Those checks are
> >> internal to pg_terminate_backend/pg_cancel_backend so GRANT is not a
> >> substitute.
> >
> > If we extend this into the future then it seems to potentially fall
> > afoul of Noah's concern that we're going to end up with two different
> > ways of saying GRANT EXECUTE X ON Y.  Consider the more complicated case
> > of pg_stat_activity, where values are shown or hidden based on who the
> > current role is.  The policy system only supports whole-row, today, but
> > the question has already come up, both on the lists and off, of support
> > for hiding individual column values for certain rows, exactly as we do
> > today for pg_stat_activity.  Once we reach that point, we'll have a way
> > to GRANT out these rights and a default role which just has them.
> 
> Well, I'm not saying that predefined rolls are the *only* way to solve
> a problem like this, but I think they're one way and I don't clearly
> see that something else is better.  However, my precise point is that
> we should *not* have predefined rolls that precisely duplicate the
> result of GRANT EXECUTE X ON Y.  Having predefined rolls that change
> the behavior of the system in other ways is a different thing.  So I
> don't see how this falls afoul of Noah's concern.  (If it does,
> perhaps he can clarify.)

Apologies if it seems like what I'm getting at is obtuse but I'm trying
to generalize this, to provide guidance on how to handle the larger set
of privileges.

If I'm following correctly, having default roles for cases where the
role is specifically for something more than 'GRANT EXECUTE X ON Y' (or
a set of such commands..?) makes sense.  Going back to the list of
roles, that would mean that default roles:

pg_monitor

  Allows roles granted more information from pg_stat_activity.  Can't be
  just a regular non-default-role right as we don't, currently, have a
  way to say "filter out the values of certain columns on certain rows,
  but not on others."

  (There's a question here though- for the privileges which will be
  directly GRANT'able, should we GRANT those to pg_monitor, or have
  pg_monitor only provide unfiltered access to pg_stat_activity, or..?
  Further, if it's only for pg_stat_activity, should we name it
  something else?)

pg_signal_backend

  Allows roles to signal other backend processes beyond those backends
  which are owned by a user they are a role member of.  Can't be a
  regular non-default-role right as we don't, currently, have any way to
  GRANT rights to send signals only to backends you own or are a member
  of.

pg_replication

  Allows roles to use the various replication functions.  Can't be a
  regular non-default-role right as the REPLICATION role attribute
  allows access to those functions and the GRANT system has no way of
  saying "allow access to these functions if they have role attribute
  X."

Make sense, as these are cases where we can't simply write GRANT
statements and get the same result, but we don't need (or at least,
shouldn't have without supporting GRANT on catalog objects and agreement
on what they're intended for):

pg_backup

  pg_start_backup(), pg_stop_backup(), pg_switch_xlog(), and
  pg_create_restore_point() will all be managed by the normal GRANT
  system and therefore we don't need a default role for those use-cases.

pg_file_settings

  pg_file_settings() function and pg_file_settings view will be managed
  by the normal GRANT system and therefore we don't need a default role
  for those use-cases.

pg_replay

  pg_xlog_replay_pause(), and pg_xlog_replay_resume() will be managed
  by the normal GRANT system and therefore we don't need a default role
  for those use-cases.

pg_rotate_logfile

  pg_rotate_logfile() will be managed by the normal GRANT system and
  therefore we don't need a default role for that use-cases.

> > Personally, I don't have any particular issue having both, but the
> > desire was stated that it would be better to have the regular
> > GRANT EXECUTE ON catalog_func() working before we consider having
> > default roles for same.  That moves the goal posts awful far though, if
> > we're to stick with that and consider how we might extend the GRANT
> > system in the future.
> 
> I don't think it moves 

[HACKERS] thanks for FOSDEM/PGDay 2016 Developer Meeting

[Oleg Bartunov]

I read  https://wiki.postgresql.org/wiki/FOSDEM/PGDay_2016_Developer_Meeting
and would like to say thanks for such nice review of meeting.

Oleg

Re: [HACKERS] CustomScan under the Gather node?

[Kouhei Kaigai]

> On Thu, Jan 28, 2016 at 10:50 AM, Kouhei Kaigai  wrote:
> >> If I would make a proof-of-concept patch with interface itself, it
> >> seems to me file_fdw may be a good candidate for this enhancement.
> >> It is not a field for postgres_fdw.
> >>
> > The attached patch is enhancement of FDW/CSP interface and PoC feature
> > of file_fdw to scan source file partially. It was smaller enhancement
> > than my expectations.
> >
> > It works as follows. This query tried to read 20M rows from a CSV file,
> > using 3 background worker processes.
> >
> > postgres=# set max_parallel_degree = 3;
> > SET
> > postgres=# explain analyze select * from test_csv where id % 20 = 6;
> >   QUERY PLAN
> >
> 
> 
> >  Gather  (cost=1000.00..194108.60 rows=94056 width=52)
> >  (actual time=0.570..19268.010 rows=200 loops=1)
> >Number of Workers: 3
> >->  Parallel Foreign Scan on test_csv  (cost=0.00..183703.00 rows=94056
> width=52)
> >   (actual time=0.180..12744.655 rows=50
> loops=4)
> >  Filter: ((id % 20) = 6)
> >  Rows Removed by Filter: 950
> >  Foreign File: /tmp/testdata.csv
> >  Foreign File Size: 1504892535
> >  Planning time: 0.147 ms
> >  Execution time: 19330.201 ms
> > (9 rows)
> 
> Could you try it not in parallel and then with 1, 2, 3, and 4 workers
> and post the times for all?
>
The above query has 5% selectivity on the entire CSV file.
Its execution time (total, only ForeignScan) are below

 total ForeignScandiff
0 workers: 17584.319 ms   17555.904 ms  28.415 ms
1 workers: 18464.476 ms   18110.968 ms 353.508 ms
2 workers: 19042.755 ms   14580.335 ms4462.420 ms
3 workers: 19318.254 ms   12668.912 ms6649.342 ms
4 workers: 21732.910 ms   13596.788 ms8136.122 ms
5 workers: 23486.846 ms   14533.409 ms8953.437 ms

This workstation has 4 CPU cores, so it is natural nworkers=3 records the
peak performance on ForeignScan portion. On the other hands, nworkers>1 also
recorded unignorable time consumption (probably, by Gather node?)

An interesting observation was, less selectivity (1% and 0%) didn't change the
result so much. Something consumes CPU time other than file_fdw.

* selectivity 1%
   total   ForeignScan   diff
0 workers: 17573.572 ms   17566.875 ms  6.697 ms
1 workers: 18098.070 ms   18020.790 ms 77.280 ms
2 workers: 18676.078 ms   14600.749 ms   4075.329 ms
3 workers: 18830.597 ms   12731.459 ms   6099.138 ms
4 workers: 21015.842 ms   13590.657 ms   7425.185 ms
5 workers: 22865.496 ms   14634.342 ms   8231.154 ms

* selectivity 0% (...so Gather didn't work hard actually)
  totalForeignScan   diff
0 workers: 17551.011 ms   17550.811 ms  0.200 ms
1 workers: 18055.185 ms   18048.975 ms  6.210 ms
2 workers: 18567.660 ms   14593.974 ms   3973.686 ms
3 workers: 18649.819 ms   12671.429 ms   5978.390 ms
4 workers: 20619.184 ms   13606.715 ms   7012.469 ms
5 workers: 22557.575 ms   14594.420 ms   7963.155 ms

Further investigation will need

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 

postgres=# explain analyze select * from test_csv where id % 100 = 100;
   QUERY PLAN
-
 Foreign Scan on test_csv  (cost=0.00..2158874.49 rows=94056 width=52) (actual 
time=17550.811..17550.811 rows=0 loops=1)
   Filter: ((id % 100) = 100)
   Rows Removed by Filter: 2000
   Foreign File: /tmp/testdata.csv
   Foreign File Size: 1504892535
 Planning time: 1.175 ms
 Execution time: 17551.011 ms
(7 rows)

postgres=# SET max_parallel_degree = 1;
SET
postgres=# explain analyze select * from test_csv where id % 100 = 100;
  QUERY PLAN
---
 Gather  (cost=1000.00..194108.60 rows=94056 width=52) (actual 
time=18054.651..18054.651 rows=0 loops=1)
   Number of Workers: 1
   ->  Parallel Foreign Scan on test_csv  (cost=0.00..183703.00 rows=94056 
width=52) (actual time=18048.975..18048.975 rows=0 loops=2)
 Filter: ((id % 100) = 100)
 Rows Removed by Filter: 2000
 Foreign File: /tmp/testdata.csv
 Foreign File Size: 1504892535
 Planning time: 0.461 ms
 Execution time: 18055.185 ms
(9 rows)

postgres=# SET max_parallel_degree = 2;
SET
postgres=# explain analyze select * from test_csv where id % 100 = 100;
  QUERY PLAN

Re: [HACKERS] [PROPOSAL] VACUUM Progress Checker.

[Amit Langote]

On 2016/01/28 23:53, Robert Haas wrote:
> On Thu, Jan 28, 2016 at 8:41 AM, Amit Langote  wrote:
>> Or keep scanned_heap_pages as is and add a skipped_pages (or
>> skipped_heap_pages). I guess the latter would be updated not only for
>> all visible skipped pages but also pin skipped pages. That is,
>> updating its counter right after vacrelstats->pinskipped_pages++ which
>> there are a couple of instances of. Likewise a good (and only?) time
>> to update the former's counter would be right after
>> vacrelstats->scanned_pages++. Although, I see at least one place where
>> both are incremented so maybe I'm not entirely correct about the last
>> two sentences.
> 
> So I've spent a fair amount of time debugging really-long-running
> VACUUM processes with customers, and generally what I really want to
> know is:
> 
 What block number are we at? <<<
> 
> Because, if I know that, and I can see how fast that's increasing,
> then I can estimate whether the VACUUM is going to end in a reasonable
> period of time or not.  So my preference is to not bother breaking out
> skipped pages, but just report the block number and call it good.  I
> will defer to a strong consensus on something else, but reporting the
> block number has the advantage of being dead simple and, in my
> experience, that would answer the question that I typically have.

Okay, I agree that reporting just the current blkno is simple and good
enough. How about numbers of what we're going to report as the "Vacuuming
Index and Heap" phase? I guess we can still keep the scanned_index_pages
and index_scan_count. So we have:

+CREATE VIEW pg_stat_vacuum_progress AS
+   SELECT
+  S.pid,
+  S.relid,
+  S.phase,
+  S.total_heap_blks,
+  S.current_heap_blkno,
+  S.total_index_pages,
+  S.scanned_index_pages,
+  S.index_scan_count
+  S.percent_complete,
+   FROM pg_stat_get_vacuum_progress() AS S;

I guess it won't remain pg_stat_get_"vacuum"_progress(), though.

Thanks,
Amit




-- 
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] CustomScan under the Gather node?

[Kouhei Kaigai]

>  total ForeignScandiff
> 0 workers: 17584.319 ms   17555.904 ms  28.415 ms
> 1 workers: 18464.476 ms   18110.968 ms 353.508 ms
> 2 workers: 19042.755 ms   14580.335 ms4462.420 ms
> 3 workers: 19318.254 ms   12668.912 ms6649.342 ms
> 4 workers: 21732.910 ms   13596.788 ms8136.122 ms
> 5 workers: 23486.846 ms   14533.409 ms8953.437 ms
> 
> This workstation has 4 CPU cores, so it is natural nworkers=3 records the
> peak performance on ForeignScan portion. On the other hands, nworkers>1 also
> recorded unignorable time consumption (probably, by Gather node?)
  :
> Further investigation will need
>
It was a bug of my file_fdw patch. ForeignScan node in the master process was
also kicked by the Gather node, however, it didn't have coordinate information
due to oversight of the initialization at InitializeDSMForeignScan callback.
In the result, local ForeignScan node is still executed after the completion
of coordinated background worker processes, and returned twice amount of rows.

In the revised patch, results seems to me reasonable.
 total ForeignScan  diff
0 workers: 17592.498 ms   17564.457 ms 28.041ms
1 workers: 12152.998 ms   11983.485 ms169.513 ms
2 workers: 10647.858 ms   10502.100 ms145.758 ms
3 workers:  9635.445 ms9509.899 ms125.546 ms
4 workers: 11175.456 ms   10863.293 ms312.163 ms
5 workers: 12586.457 ms   12279.323 ms307.134 ms

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


> -Original Message-
> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Kouhei Kaigai
> Sent: Friday, January 29, 2016 8:51 AM
> To: Robert Haas
> Cc: pgsql-hackers@postgresql.org
> Subject: Re: [HACKERS] CustomScan under the Gather node?
> 
> > On Thu, Jan 28, 2016 at 10:50 AM, Kouhei Kaigai  
> > wrote:
> > >> If I would make a proof-of-concept patch with interface itself, it
> > >> seems to me file_fdw may be a good candidate for this enhancement.
> > >> It is not a field for postgres_fdw.
> > >>
> > > The attached patch is enhancement of FDW/CSP interface and PoC feature
> > > of file_fdw to scan source file partially. It was smaller enhancement
> > > than my expectations.
> > >
> > > It works as follows. This query tried to read 20M rows from a CSV file,
> > > using 3 background worker processes.
> > >
> > > postgres=# set max_parallel_degree = 3;
> > > SET
> > > postgres=# explain analyze select * from test_csv where id % 20 = 6;
> > >   QUERY PLAN
> > >
> >
> 
> > 
> > >  Gather  (cost=1000.00..194108.60 rows=94056 width=52)
> > >  (actual time=0.570..19268.010 rows=200 loops=1)
> > >Number of Workers: 3
> > >->  Parallel Foreign Scan on test_csv  (cost=0.00..183703.00 rows=94056
> > width=52)
> > >   (actual time=0.180..12744.655
> rows=50
> > loops=4)
> > >  Filter: ((id % 20) = 6)
> > >  Rows Removed by Filter: 950
> > >  Foreign File: /tmp/testdata.csv
> > >  Foreign File Size: 1504892535
> > >  Planning time: 0.147 ms
> > >  Execution time: 19330.201 ms
> > > (9 rows)
> >
> > Could you try it not in parallel and then with 1, 2, 3, and 4 workers
> > and post the times for all?
> >
> The above query has 5% selectivity on the entire CSV file.
> Its execution time (total, only ForeignScan) are below
> 
>  total ForeignScandiff
> 0 workers: 17584.319 ms   17555.904 ms  28.415 ms
> 1 workers: 18464.476 ms   18110.968 ms 353.508 ms
> 2 workers: 19042.755 ms   14580.335 ms4462.420 ms
> 3 workers: 19318.254 ms   12668.912 ms6649.342 ms
> 4 workers: 21732.910 ms   13596.788 ms8136.122 ms
> 5 workers: 23486.846 ms   14533.409 ms8953.437 ms
> 
> This workstation has 4 CPU cores, so it is natural nworkers=3 records the
> peak performance on ForeignScan portion. On the other hands, nworkers>1 also
> recorded unignorable time consumption (probably, by Gather node?)
> 
> An interesting observation was, less selectivity (1% and 0%) didn't change the
> result so much. Something consumes CPU time other than file_fdw.
> 
> * selectivity 1%
>total   ForeignScan   diff
> 0 workers: 17573.572 ms   17566.875 ms  6.697 ms
> 1 workers: 18098.070 ms   18020.790 ms 77.280 ms
> 2 workers: 18676.078 ms   14600.749 ms   4075.329 ms
> 3 workers: 18830.597 ms   12731.459 ms   6099.138 ms
> 4 workers: 21015.842 ms   13590.657 ms   7425.185 ms
> 5 workers: 22865.496 ms   14634.342 ms   8231.154 ms
> 
> * selectivity 0% (...so Gather didn't work hard actually)
>   totalForeignScan   diff
> 0 workers: 17551.011 ms   17550.811 ms  0.200 ms
> 1 workers: 18055.185 ms   18048.975 ms  6.210 ms
> 2 

Re: [HACKERS] thanks for FOSDEM/PGDay 2016 Developer Meeting

[Michael Paquier]

On Fri, Jan 29, 2016 at 6:18 AM, Amit Langote  wrote:
> On Friday, 29 January 2016, Oleg Bartunov  wrote:
>> I read
>> https://wiki.postgresql.org/wiki/FOSDEM/PGDay_2016_Developer_Meeting and
>> would like to say thanks for such nice review of meeting.
>
>
> +many

+1. Thanks for taking the time to type and publish it.
-- 
Michael


-- 
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] New committer

[Michael Paquier]

On Fri, Jan 29, 2016 at 1:42 AM, Peter Geoghegan  wrote:
> On Thu, Jan 28, 2016 at 6:37 AM, Magnus Hagander  wrote:
>> The PostgreSQL core team would like to welcome Dean Rasheed as a new
>> committer for the PostgreSQL project.
>
> Congratulations, Dean. Well deserved.

Congratulations, Dean!
-- 
Michael


-- 
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] Template for commit messages

[Alvaro Herrera]

Stephen Frost wrote:

> > OK, but keep in mind whatever script committers user should remove tags
> > that are empty after exiting the editor.  I can provide the grep regex
> > in git somewhere too:
> > 
> >   egrep -v 
> > "^(Author|Reported-by|Bug|Reviewed-by|Tested-by|Backpatch-through): *$"
> 
> If the template is there then, for my part at least, I wouldn't mind
> killing the empty lines.  Having a decent script would work too, of
> course... but I have to admit that I've not tried having a script modify
> my commit messages right before they're committed and, well, it'd take a
> bit for me to be comfortable with it.
> 
> No one wants garbled commit messages from a script that went awry. ;)

Maybe it'd be better to have the lines start with a # , because then
git commit itself removes those as comments.  So the committer would
need to remove the # and then fill in the data for the field.

git-commit(1) says:

   -t , --template=
   When editing the commit message, start the editor with the contents
   in the given file. The commit.template configuration variable is
   often used to give this option implicitly to the command. This
   mechanism can be used by projects that want to guide participants
   with some hints on what to write in the message in what order. If
   the user exits the editor without editing the message, the commit
   is aborted. This has no effect when a message is given by other
   means, e.g. with the -m or -F options.

-- 
Á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] Template for commit messages

[Andrew Dunstan]

On 01/28/2016 09:57 AM, Robert Haas wrote:

On Thu, Jan 28, 2016 at 9:52 AM, Tom Lane  wrote:

Robert Haas  writes:

On Thu, Jan 28, 2016 at 8:04 AM, Tomas Vondra
 wrote:

Why can't we do both? That is, have a free-form text with the nuances, and
then Reviewed-By listing the main reviewers? The first one is for humans,
the other one for automated tools.

I'm not objecting to or endorsing any specific proposal, just asking
what we want to do about this.  I think the trick if we do it that way
will be to avoid having it seem like too much duplication, but there
may be a way to manage that.

FWIW, I'm a bit suspicious of the idea that we need to make the commit
messages automated-tool-friendly.  What tools are there that would need
to extract this info, and would we trust them if they didn't understand
"nuances"?

I'm on board with Bruce's template as being a checklist of points to be
sure to cover when composing a commit message.  I'm not sure we need
fixed-format rules.

Well, I think what people are asking for is precisely a fixed format,
and I do think there is value in that.  It's nice to capture the
nuance, but the nuance is going to get flattened out anyway when the
release notes are created.  If we all agree to use a fixed format,
then a bunch of work there that probably has to be done manually can
be automated.  However, if we all agree to use a fixed format except
for you, we might as well just forget the whole thing, because the
percentage of commits that are done by you is quite high.




Yeah.

I have no prejudice in this area, other than one in favor of any rules 
being fairly precise. As for nuances, I guess they can be specified in 
the commit message. The one thing I do find annoying from time to time 
is the limit on subject size. Sometimes it's very difficult to be 
sufficiently communicative in, say, 50 characters.


cheers

andrew


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

Re: [HACKERS] extend pgbench expressions with functions

[Fabien COELHO]

I do not think that it is really worth fixing, but I will not prevent anyone
to fix it.


I still think it does. Well, if there is consensus to address this one
and optionally the other integer overflows even on back branches, I'll
write a patch and let's call that a deal. This is not a problem from
my side.


My point is just about the cost-benefit of fixing a low probability issue 
that you can only encounter if you are looking for it, and not with any 
reasonable bench script.


Now adding somewhere a test might just help closing the subject and 
do more useful things, so that would also be a win.


  /* these would raise an arithmetic error */
  if (lval == INT64_MIN && rval == -1)
  {
 fprintf(stderr, "cannot divide or modulo INT64_MIN by -1\n");
 return false;
  }

This may be backpatched to old supported versions.

--
Fabien.


--
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] Trivial doc fix in logicaldecoding.sgml

[Craig Ringer]

On 28 January 2016 at 16:36, Shulgin, Oleksandr <
oleksandr.shul...@zalando.de> wrote:

> On Thu, Jan 28, 2016 at 5:55 AM, Fujii Masao 
> wrote:
>
>> On Wed, Jan 27, 2016 at 7:34 PM, Shulgin, Oleksandr
>>  wrote:
>> > Hi,
>> >
>> > Please find attached a simple copy-paste fix for CREATE_REPLICATION_SLOT
>> > syntax.
>>
>> We should change also START_REPLICATION SLOT syntax document as follows?
>>
>> -  START_REPLICATION SLOT
>> slot_name LOGICAL
>> options
>> +  START_REPLICATION SLOT
>> slot_name LOGICAL
>> XXX/XXX
>> (options)
>>
>
> If a committer would thinks so, I don't object.  Though this one is rather
> a detail for which the reader is already referred to protocol-replication,
> while my fix was about a factual error.
>
>
I think it should be changed. I've already had people confused by this.

Either that or remove the synopsis entirely, changing it to

START_REPLICATION SLOT 

and linking to the protocol docs. Which might be better.



-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

[HACKERS] Template for commit messages

[Bruce Momjian]

There has been a request in the FOSDEM developers meeting that
committers use a more consistent format for commit messages.  This is
the format I use:

-- email subject limit -
-- gitweb summary limit --


Report by

Patch by

Reviewed by

Backpatch through

The dashed lines are used to specify a target length for the summary
line and are automatically removed before the commit message is posted.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


-- 
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] better systemd integration

[Pavel Stehule]

>
> > Second issue:
> >
> > Mapping of levels between pg and journal levels is moved by1
>
> This is the same as how the "syslog" destination works.
>
>

I understand to this logic, but I miss any documentation.

Regards

Pavel

Re: [HACKERS] Declarative partitioning

[Amit Langote]

Hi Tomas,

Thanks for your comments and sorry for replying so late.

On 2016/01/22 22:54, Tomas Vondra wrote:
> thanks for working on this. Seems the last version of the patch was
> submitted more than 2 months ago and I believe large parts of it will get
> reworked based on the extensive discussion on this list, so I haven't
> looked at the code at all.
> 
> I'd like to comment on the one thing and that's the syntax. It seems to me
> we're really trying to reinvent the wheel and come up with our own version
> of the syntax. Is there a particular reason why not to look at the syntax
> of the other databases and adapt as much of the existing syntax as possible?
> 
> I think that's for a few reasons - firstly it makes the life much easier
> for the DBAs and users who are either migrating to PostgreSQL or have to
> manage a mix of databases. Secondly, it serves as a valuable source of
> engineering info, preventing the "I haven't thought of this use case"
> problem.
> 
> An example of this is the proposed syntax for adding a partition
> 
>   CREATE TABLE measurement_fail
>   PARTITION OF measurement
>   FOR VALUES START ('2006-02-15') END ('2006-03-01');
> 
> which seems a bit awkward as both the databases I'm familiar with (Oracle
> and Sybase) use ALTER TABLE to do this
> 
>   ALTER TABLE measurement
> ADD PARTITION measurement_fail VALUES LESS THAN ( ... )

Syntax like the one you mention allows to create/modify/move/drop
partitions at 2 levels (generally) using PARTITION and SUBPARTITION
keywords. That might be limiting to some users. I don't have a clear
picture of what a syntax that's general enough would look like, but I
proposed something like what follows:

CREATE TABLE parent (
  a int,
  b char(10)
) PARTITION BY RANGE ON (a)
  SUBPARTITION BY LIST ON ((substring(b from 1 for 2)));

CREATE PARTITION partname OF parent FOR VALUES LESS THAN (100);
CREATE PARTITION subpartname OF partname FOR VALUES IN ('ab');

The latter of the CREATE PARTITION commands lets us create the so-called
sub-partition of 'parent'. Remember that in this scheme, all level 1
partitions are not actually physical tables themselves; only level 2
partitions are. If you stick one more SUBPARTITION BY in parent's
definition, you can:

CREATE PARTITION subsubpartname OF subpartname FOR VALUES ...;

This is something that the Oracle-like syntax won't be able to support.
Although, if we all agree that we'd never want to support such a case then
let's implement something that's familiar viz. the following:

CREATE TABLE parent (
  a int,
  b char(10)
) PARTITION BY RANGE ON (a)
  SUBPARTITION BY LIST ON ((substring(b from 1 for 2)));

ALTER TABLE parent
  ADD PARTITION partname FOR VALUES LESS THAN (100);

ALTER TABLE parent
  MODIFY PARTITION partname ADD SUBPARTITION subpartname FOR VALUES IN ('ab');

ALTER TABLE parent
  MODIFY PARTITION partname DROP SUBPARTITION subpartname;

ALTER TABLE parent
  DROP PARTITION partname;

> And so on for the other commands.
> 
> That being said, I entirely agree with Simon (and others) that getting the
> planner part work is the crucial part of the patch. But I also think that
> a proper abstraction (thanks to good syntax) may be a valuable hint how to
> define the catalogs and such.

I tried to do that in the November commitfest but decided to just work on
the syntax as it became clear that throwing too many changes at the
reviewers/committers like that may not be such a great idea. Syntax itself
is a big enough change to discuss and reach consensus on. Let's get the
syntax, catalog and some infrastructure for basic features of the new
partitioning to work. We can think about planner enhancements for
partitioned tables that are waiting for declarative partitioning to get in
later.

Thanks,
Amit




-- 
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] Trivial doc fix in logicaldecoding.sgml

[Shulgin, Oleksandr]

On Thu, Jan 28, 2016 at 5:55 AM, Fujii Masao  wrote:

> On Wed, Jan 27, 2016 at 7:34 PM, Shulgin, Oleksandr
>  wrote:
> > Hi,
> >
> > Please find attached a simple copy-paste fix for CREATE_REPLICATION_SLOT
> > syntax.
>
> We should change also START_REPLICATION SLOT syntax document as follows?
>
> -  START_REPLICATION SLOT
> slot_name LOGICAL
> options
> +  START_REPLICATION SLOT
> slot_name LOGICAL
> XXX/XXX
> (options)
>

If a committer would thinks so, I don't object.  Though this one is rather
a detail for which the reader is already referred to protocol-replication,
while my fix was about a factual error.

--
Alex

Re: [HACKERS] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Alvaro Herrera]

Etsuro Fujita wrote:
> On 2016/01/28 12:13, Robert Haas wrote:

> >I don't think this is a good idea.  Most of the time, no system
> >columns will be present, and we'll just be scanning the Bitmapset
> >twice rather than once.  Sure, that doesn't take many extra cycles,
> >but if the point of all this is to micro-optimize this code, that
> >particular change is going in the wrong direction.
> 
> I thought that is a good idea, considering the additional overhead is
> little, because that would be useful for some use-cases.  But I think we
> need more discussions about that, so if there are no objections from Alvaro
> (or anyone), I'll leave that part as-is.

I'm happy to defer.

-- 
Á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] [PATCH] better systemd integration

[Pavel Stehule]

Hi

2016-01-28 3:50 GMT+01:00 Peter Eisentraut :

> On 1/27/16 7:02 AM, Pavel Stehule wrote:
> > The issues:
> >
> > 1. configure missing systemd integration test, compilation fails:
> >
> > postmaster.o postmaster.c
> > postmaster.c:91:31: fatal error: systemd/sd-daemon.h: No such file or
> > directory
>
> Updated patch attached that fixes this by adding additional checking in
> configure.
>


You sent only rebased code of previous version. I didn't find additional
checks.


>
> > 3. PostgreSQL is able to write to systemd log, but multiline entry was
> > stored with different priorities
>
> Yeah, as Tom had already pointed out, this doesn't work as I had
> imagined it.  I'm withdrawing this part of the patch for now.  I'll come
> back to it later.
>

ok


>
> > Second issue:
> >
> > Mapping of levels between pg and journal levels is moved by1
>
> This is the same as how the "syslog" destination works.
>
>
I didn't find any related code in PostgreSQL, can me help, please?

Regards

Pavel

Re: [HACKERS] Trivial doc fix in logicaldecoding.sgml

[Shulgin, Oleksandr]

On Thu, Jan 28, 2016 at 9:42 AM, Craig Ringer  wrote:
>
>
> On 28 January 2016 at 16:36, Shulgin, Oleksandr <
oleksandr.shul...@zalando.de> wrote:
>>
>> On Thu, Jan 28, 2016 at 5:55 AM, Fujii Masao 
wrote:
>>>
>>>
>>> We should change also START_REPLICATION SLOT syntax document as follows?
>>>
>>> -  START_REPLICATION SLOT
>>> slot_name LOGICAL
>>> options
>>> +  START_REPLICATION SLOT
>>> slot_name LOGICAL
>>> XXX/XXX
>>> (options)
>>
>>
>> If a committer would thinks so, I don't object.  Though this one is
rather a detail for which the reader is already referred to
protocol-replication, while my fix was about a factual error.
>>
>
> I think it should be changed. I've already had people confused by this.
>
> Either that or remove the synopsis entirely, changing it to
>
> START_REPLICATION SLOT 
>
> and linking to the protocol docs. Which might be better.

I think it still makes sense to keep the LOGICAL, but hide the rest of the
details behind that ellipsis, so:

  START_REPLICATION SLOT slot_name LOGICAL ...

Updated patch attached.

--
Alex
From 11b61d04cd16b9577759edff15e68f8ba9c4828e Mon Sep 17 00:00:00 2001
From: Oleksandr Shulgin 
Date: Wed, 27 Jan 2016 11:27:35 +0100
Subject: [PATCH] Fix protocol commands description in logicaldecoding.sgml

---
 doc/src/sgml/logicaldecoding.sgml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/logicaldecoding.sgml b/doc/src/sgml/logicaldecoding.sgml
index 1ae5eb6..e841348 100644
--- a/doc/src/sgml/logicaldecoding.sgml
+++ b/doc/src/sgml/logicaldecoding.sgml
@@ -280,7 +280,7 @@ $ pg_recvlogical -d postgres --slot test --drop-slot
 The commands
 
  
-  CREATE_REPLICATION_SLOT slot_name LOGICAL options
+  CREATE_REPLICATION_SLOT slot_name LOGICAL output_plugin
  
 
  
@@ -288,7 +288,7 @@ $ pg_recvlogical -d postgres --slot test --drop-slot
  
 
  
-  START_REPLICATION SLOT slot_name LOGICAL options
+  START_REPLICATION SLOT slot_name LOGICAL ...
  
 
 are used to create, drop, and stream changes from a replication
-- 
2.5.0


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

[HACKERS] Several problems in tab-completions for SET/RESET

[Fujii Masao]

Hi,

I found that the following tab-completions for SET/RESET which
worked properly before doesn't work properly now in the master.

1. ALTER SYSTEM SET|RESET  lists nothing.
2. ALTER DATABASE xxx SET  lists nothing.
3. ALTER DATABASE xxx SET yyy  lists nothing.
4. ALTER DATABASE xxx SET datestyle TO  lists nothing.

Attached patch fixes those problems.

Regards,

-- 
Fujii Masao
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
***
*** 1553,1559  psql_completion(const char *text, int start, int end)
  	else if (Matches2("ALTER", "SYSTEM"))
  		COMPLETE_WITH_LIST2("SET", "RESET");
  	/* ALTER SYSTEM SET|RESET  */
! 	else if (Matches4("ALTER", "SYSTEM", "SET|RESET", MatchAny))
  		COMPLETE_WITH_QUERY(Query_for_list_of_alter_system_set_vars);
  	/* ALTER VIEW  */
  	else if (Matches3("ALTER", "VIEW", MatchAny))
--- 1553,1559 
  	else if (Matches2("ALTER", "SYSTEM"))
  		COMPLETE_WITH_LIST2("SET", "RESET");
  	/* ALTER SYSTEM SET|RESET  */
! 	else if (Matches3("ALTER", "SYSTEM", "SET|RESET"))
  		COMPLETE_WITH_QUERY(Query_for_list_of_alter_system_set_vars);
  	/* ALTER VIEW  */
  	else if (Matches3("ALTER", "VIEW", MatchAny))
***
*** 2702,2708  psql_completion(const char *text, int start, int end)
  
  /* SET, RESET, SHOW */
  	/* Complete with a variable name */
! 	else if (Matches1("SET|RESET"))
  		COMPLETE_WITH_QUERY(Query_for_list_of_set_vars);
  	else if (Matches1("SHOW"))
  		COMPLETE_WITH_QUERY(Query_for_list_of_show_vars);
--- 2702,2708 
  
  /* SET, RESET, SHOW */
  	/* Complete with a variable name */
! 	else if (TailMatches1("SET|RESET") && !TailMatches3("UPDATE", MatchAny, "SET"))
  		COMPLETE_WITH_QUERY(Query_for_list_of_set_vars);
  	else if (Matches1("SHOW"))
  		COMPLETE_WITH_QUERY(Query_for_list_of_show_vars);
***
*** 2741,2750  psql_completion(const char *text, int start, int end)
  	else if (Matches2("RESET", "SESSION"))
  		COMPLETE_WITH_CONST("AUTHORIZATION");
  	/* Complete SET  with "TO" */
! 	else if (Matches2("SET", MatchAny))
  		COMPLETE_WITH_CONST("TO");
  	/* Suggest possible variable values */
! 	else if (Matches3("SET", MatchAny, "TO|="))
  	{
  		/* special cased code for individual GUCs */
  		if (TailMatches2("DateStyle", "TO|="))
--- 2741,2754 
  	else if (Matches2("RESET", "SESSION"))
  		COMPLETE_WITH_CONST("AUTHORIZATION");
  	/* Complete SET  with "TO" */
! 	else if (TailMatches2("SET", MatchAny) &&
! 			 !TailMatches4("UPDATE|DOMAIN", MatchAny, MatchAny, MatchAny) &&
! 			 !TailMatches1("TABLESPACE|SCHEMA") &&
! 			 !ends_with(prev_wd, ')') &&
! 			 !ends_with(prev_wd, '='))
  		COMPLETE_WITH_CONST("TO");
  	/* Suggest possible variable values */
! 	else if (TailMatches3("SET", MatchAny, "TO|="))
  	{
  		/* special cased code for individual GUCs */
  		if (TailMatches2("DateStyle", "TO|="))

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

Re: Odd behavior in foreign table modification (Was: Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW)

[Etsuro Fujita]

On 2016/01/28 12:58, Robert Haas wrote:

On Thu, Jan 21, 2016 at 4:05 AM, Etsuro Fujita
 wrote:

By the way, I'm not too sure I understand the need for the core
changes that are part of this patch, and I think that point merits
some discussion.  Whenever you change core like this, you're changing
the contract between the FDW and core; it's not just postgres_fdw that
needs updating, but every FDW.  So we'd better be pretty sure we need
these changes and they are adequately justified before we think about
putting them into the tree.  Are these core changes really needed
here, or can we fix this whole issue in postgres_fdw and leave the
core code alone?



Well, if we think it is the FDW's responsibility to insert a valid value for
tableoid in the returned slot during ExecForeignInsert, ExecForeignUpdate or
ExecForeignDelete, we don't need those core changes.  However, I think it
would be better that that's done by ModifyTable in the same way as
ForeignScan does in ForeignNext, IMO. That eliminates the need for
postgres_fdw or any other FDW to do that business in the callback routines.



I'm not necessarily opposed to the core changes, but I want to
understand better what complexity they are avoiding.  Can you send a
version of this patch that only touches postgres_fdw, so I can
compare?


Attached is that version of the patch.

I think that postgres_fdw might be able to insert a tableoid value in 
the returned slot in e.g., postgresExecForeignInsert if AFTER ROW 
Triggers or RETURNING expressions reference that value, but I didn't do 
anything about that.


Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***
*** 110,115  static void deparseTargetList(StringInfo buf,
--- 110,116 
    PlannerInfo *root,
    Index rtindex,
    Relation rel,
+   bool is_returning,
    Bitmapset *attrs_used,
    List **retrieved_attrs);
  static void deparseReturningList(StringInfo buf, PlannerInfo *root,
***
*** 724,730  deparseSelectSql(StringInfo buf,
  	 * Construct SELECT list
  	 */
  	appendStringInfoString(buf, "SELECT ");
! 	deparseTargetList(buf, root, baserel->relid, rel, attrs_used,
  	  retrieved_attrs);
  
  	/*
--- 725,731 
  	 * Construct SELECT list
  	 */
  	appendStringInfoString(buf, "SELECT ");
! 	deparseTargetList(buf, root, baserel->relid, rel, false, attrs_used,
  	  retrieved_attrs);
  
  	/*
***
*** 738,744  deparseSelectSql(StringInfo buf,
  
  /*
   * Emit a target list that retrieves the columns specified in attrs_used.
!  * This is used for both SELECT and RETURNING targetlists.
   *
   * The tlist text is appended to buf, and we also create an integer List
   * of the columns being retrieved, which is returned to *retrieved_attrs.
--- 739,746 
  
  /*
   * Emit a target list that retrieves the columns specified in attrs_used.
!  * This is used for both SELECT and RETURNING targetlists; the is_returning
!  * parameter is true only for a RETURNING targetlist.
   *
   * The tlist text is appended to buf, and we also create an integer List
   * of the columns being retrieved, which is returned to *retrieved_attrs.
***
*** 748,753  deparseTargetList(StringInfo buf,
--- 750,756 
    PlannerInfo *root,
    Index rtindex,
    Relation rel,
+   bool is_returning,
    Bitmapset *attrs_used,
    List **retrieved_attrs)
  {
***
*** 777,782  deparseTargetList(StringInfo buf,
--- 780,787 
  		{
  			if (!first)
  appendStringInfoString(buf, ", ");
+ 			else if (is_returning)
+ appendStringInfoString(buf, " RETURNING ");
  			first = false;
  
  			deparseColumnRef(buf, rtindex, i, root);
***
*** 794,799  deparseTargetList(StringInfo buf,
--- 799,806 
  	{
  		if (!first)
  			appendStringInfoString(buf, ", ");
+ 		else if (is_returning)
+ 			appendStringInfoString(buf, " RETURNING ");
  		first = false;
  
  		appendStringInfoString(buf, "ctid");
***
*** 803,809  deparseTargetList(StringInfo buf,
  	}
  
  	/* Don't generate bad syntax if no undropped columns */
! 	if (first)
  		appendStringInfoString(buf, "NULL");
  }
  
--- 810,816 
  	}
  
  	/* Don't generate bad syntax if no undropped columns */
! 	if (first && !is_returning)
  		appendStringInfoString(buf, "NULL");
  }
  
***
*** 1022,1032  deparseReturningList(StringInfo buf, PlannerInfo *root,
  	}
  
  	if (attrs_used != NULL)
! 	{
! 		appendStringInfoString(buf, " RETURNING ");
! 		deparseTargetList(buf, root, rtindex, rel, attrs_used,
  		  retrieved_attrs);
- 	}
  	else
  		*retrieved_attrs = NIL;
  }
--- 1029,1036 
  	}
  
  	if (attrs_used != NULL)
! 		deparseTargetList(buf, root, rtindex, rel, true, attrs_used,
  		  retrieved_attrs);
  	else
  		*retrieved_attrs = NIL;
  }
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- 

[HACKERS] Comment typos in source code: s/thats/that is/

[Michael Paquier]

Hi all,

I found a couple of typos as per the $subject.
A patch is attached.
Regards,
-- 
Michael
diff --git a/contrib/pgcrypto/fortuna.c b/contrib/pgcrypto/fortuna.c
index 7ab888f..5028203 100644
--- a/contrib/pgcrypto/fortuna.c
+++ b/contrib/pgcrypto/fortuna.c
@@ -304,7 +304,7 @@ get_rand_pool(FState *st)
 	unsigned	rnd;
 
 	/*
-	 * This slightly prefers lower pools - thats OK.
+	 * This slightly prefers lower pools - that is OK.
 	 */
 	rnd = st->key[st->rnd_pos] % NUM_POOLS;
 
diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.c
index 010b5fc..4ff4caf 100644
--- a/src/backend/postmaster/bgwriter.c
+++ b/src/backend/postmaster/bgwriter.c
@@ -301,7 +301,7 @@ BackgroundWriterMain(void)
 		 * check whether there has been any WAL inserted since the last time
 		 * we've logged a running xacts.
 		 *
-		 * We do this logging in the bgwriter as its the only process thats
+		 * We do this logging in the bgwriter as its the only process that is
 		 * run regularly and returns to its mainloop all the time. E.g.
 		 * Checkpointer, when active, is barely ever in its mainloop and thus
 		 * makes it hard to log regularly.
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index 651f53f..757b50e 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -677,7 +677,7 @@ StartupReplicationOrigin(void)
  errmsg("could not open file \"%s\": %m",
 		path)));
 
-	/* verify magic, thats written even if nothing was active */
+	/* verify magic, that is written even if nothing was active */
 	readBytes = read(fd, , sizeof(magic));
 	if (readBytes != sizeof(magic))
 		ereport(PANIC,
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 97c1ad4..ed823ec 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -612,7 +612,7 @@ SnapBuildExportSnapshot(SnapBuild *builder)
 void
 SnapBuildClearExportedSnapshot(void)
 {
-	/* nothing exported, thats the usual case */
+	/* nothing exported, that is the usual case */
 	if (!ExportInProgress)
 		return;
 
diff --git a/src/common/pg_lzcompress.c b/src/common/pg_lzcompress.c
index 5750477..985841d 100644
--- a/src/common/pg_lzcompress.c
+++ b/src/common/pg_lzcompress.c
@@ -153,7 +153,7 @@
  *  to be coded into a tag.
  *
  *			Finally the match algorithm checks that at least a match
- *			of 3 or more bytes has been found, because thats the smallest
+ *			of 3 or more bytes has been found, because that is the smallest
  *			amount of copy information to code into a tag. If so, a tag
  *			is omitted and all the input bytes covered by that are just
  *			scanned for the history add's, otherwise a literal character

-- 
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] Template for commit messages

[Tomas Vondra]

On 01/28/2016 01:57 PM, Robert Haas wrote:
...

One of the things I like about the current free-form approach is that
you can indicate nuances, like:

Person X reviewed an earlier version of this patch that was a lot
different than this one.
Person X reviewed this patch but didn't totally endorse it.
Person X wrote the documentation for the patch, but none of the code.
Person X wrote the vast bulk of this patch, even though there are some
other authors.

Should I just abandon the idea of trying to capture those details, or
does this format contemplate a way to include them?


Why can't we do both? That is, have a free-form text with the nuances, 
and then Reviewed-By listing the main reviewers? The first one is for 
humans, the other one for automated tools.




(Also an important question: Has Tom agreed to use this new format?
Because I think that anything the rest of agree on that he's not
prepared to endorse is not going to have much value.)



I can't speak for Tom, but I'm sitting fairly close to him and I haven't 
heard any complains or even groaning.


regards

--
Tomas Vondra  http://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:Use PGDLLEXPORT for libpq

[Michael Paquier]

On Thu, Jan 28, 2016 at 8:51 PM, Yury Zhuravlev
 wrote:
> Craig Ringer wrote:
>>
>> I strongly disagree. MSVC is a high quality compiler and the primary tool
>> for the platform.
>
> Ok. And we not suport MSVC2015 now. Either we support the platform normally
> or throwing it. Now it all looks like a zombie.

Well, 2015 is not making things easy visibly by not providing direct
ways to get locale information.
-- 
Michael


-- 
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] Template for commit messages

[Robert Haas]

On Thu, Jan 28, 2016 at 3:52 AM, Bruce Momjian  wrote:
> There has been a request in the FOSDEM developers meeting that
> committers use a more consistent format for commit messages.  This is
> the format I use:
>
> -- email subject limit -
> -- gitweb summary limit --
>
>
> Report by
>
> Patch by
>
> Reviewed by
>
> Backpatch through
>
> The dashed lines are used to specify a target length for the summary
> line and are automatically removed before the commit message is posted.

One of the things I like about the current free-form approach is that
you can indicate nuances, like:

Person X reviewed an earlier version of this patch that was a lot
different than this one.
Person X reviewed this patch but didn't totally endorse it.
Person X wrote the documentation for the patch, but none of the code.
Person X wrote the vast bulk of this patch, even though there are some
other authors.

Should I just abandon the idea of trying to capture those details, or
does this format contemplate a way to include them?

(Also an important question: Has Tom agreed to use this new format?
Because I think that anything the rest of agree on that he's not
prepared to endorse is not going to have much value.)

-- 
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] Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

[Etsuro Fujita]

On 2016/01/28 18:15, Alvaro Herrera wrote:

Etsuro Fujita wrote:

On 2016/01/28 12:13, Robert Haas wrote:



I don't think this is a good idea.  Most of the time, no system
columns will be present, and we'll just be scanning the Bitmapset
twice rather than once.  Sure, that doesn't take many extra cycles,
but if the point of all this is to micro-optimize this code, that
particular change is going in the wrong direction.



I thought that is a good idea, considering the additional overhead is
little, because that would be useful for some use-cases.  But I think we
need more discussions about that, so if there are no objections from Alvaro
(or anyone), I'll leave that part as-is.



I'm happy to defer.


Done.  Attached is an updated version of the patch.

Best regards,
Etsuro Fujita
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***
*** 2099,2108  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	RelOptInfo *rel = best_path->path.parent;
  	Index		scan_relid = rel->relid;
  	Oid			rel_oid = InvalidOid;
- 	Bitmapset  *attrs_used = NULL;
  	Plan	   *outer_plan = NULL;
- 	ListCell   *lc;
- 	int			i;
  
  	Assert(rel->fdwroutine != NULL);
  
--- 2099,2105 
***
*** 2171,2206  create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
  	}
  
  	/*
! 	 * Detect whether any system columns are requested from rel.  This is a
! 	 * bit of a kluge and might go away someday, so we intentionally leave it
! 	 * out of the API presented to FDWs.
! 	 *
! 	 * First, examine all the attributes needed for joins or final output.
! 	 * Note: we must look at reltargetlist, not the attr_needed data, because
! 	 * attr_needed isn't computed for inheritance child rels.
  	 */
! 	pull_varattnos((Node *) rel->reltargetlist, rel->relid, _used);
! 
! 	/* Add all the attributes used by restriction clauses. */
! 	foreach(lc, rel->baserestrictinfo)
  	{
! 		RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
  
! 		pull_varattnos((Node *) rinfo->clause, rel->relid, _used);
! 	}
  
! 	/* Now, are any system columns requested from rel? */
! 	scan_plan->fsSystemCol = false;
! 	for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
! 	{
! 		if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
  		{
! 			scan_plan->fsSystemCol = true;
! 			break;
  		}
- 	}
  
! 	bms_free(attrs_used);
  
  	return scan_plan;
  }
--- 2168,2215 
  	}
  
  	/*
! 	 * If rel is a base relation, detect whether any system columns are
! 	 * requested from the rel.  (If rel is a join relation, rel->relid will be
! 	 * 0, but there can be no Var with relid 0 in the reltargetlist or the
! 	 * restriction clauses, so we skip this in that case.  Note that any such
! 	 * columns in base relations that were joined are assumed to be contained
! 	 * in fdw_scan_tlist.)  This is a bit of a kluge and might go away someday,
! 	 * so we intentionally leave it out of the API presented to FDWs.
  	 */
! 	scan_plan->fsSystemCol = false;
! 	if (scan_relid > 0)
  	{
! 		Bitmapset  *attrs_used = NULL;
! 		ListCell   *lc;
! 		int			i;
  
! 		/*
! 		 * First, examine all the attributes needed for joins or final output.
! 		 * Note: we must look at reltargetlist, not the attr_needed data,
! 		 * because attr_needed isn't computed for inheritance child rels.
! 		 */
! 		pull_varattnos((Node *) rel->reltargetlist, scan_relid, _used);
  
! 		/* Add all the attributes used by restriction clauses. */
! 		foreach(lc, rel->baserestrictinfo)
  		{
! 			RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
! 
! 			pull_varattnos((Node *) rinfo->clause, scan_relid, _used);
  		}
  
! 		/* Now, are any system columns requested from rel? */
! 		for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
! 		{
! 			if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
! 			{
! scan_plan->fsSystemCol = true;
! break;
! 			}
! 		}
! 
! 		bms_free(attrs_used);
! 	}
  
  	return scan_plan;
  }

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

[Masahiko Sawada]

On Thu, Jan 28, 2016 at 8:05 PM, Fujii Masao  wrote:
> On Wed, Jan 20, 2016 at 2:35 PM, Masahiko Sawada  
> wrote:
>> On Tue, Jan 19, 2016 at 1:52 AM, Thom Brown  wrote:
>>> On 3 January 2016 at 13:26, Masahiko Sawada  wrote:
 On Fri, Dec 25, 2015 at 7:21 AM, Thomas Munro
  wrote:
> On Fri, Dec 25, 2015 at 8:50 AM, Masahiko Sawada  
> wrote:
>> On Wed, Dec 23, 2015 at 8:45 AM, Thomas Munro
>>  wrote:
>>> On Wed, Dec 23, 2015 at 3:50 PM, Thomas Munro
>>>  wrote:
 If you got rid of SyncRepGetSyncStandbysOnePriority as suggested
 above, then this function could be renamed to SyncRepGetSyncStandbys.
 I think it would be a tiny bit nicer if it also took a Size n argument
 along with the output buffer pointer.
>>
>> Sorry, I could not get your point. SyncRepGetSyncStandbysPriority()
>> function uses synchronous_standby_num which is global variable.
>> But you mean that the number of synchronous standbys is given as
>> function argument?
>
> Yeah, I was thinking of it as the output buffer size which I would be
> inclined to make more explicit (I am still coming to terms with the
> use of global variables in Postgres) but it doesn't matter, please
> disregard that suggestion.
>
 As for the body of that function (which I won't paste here), it
 contains an algorithm to find the top K elements in an array of N
 elements.  It does that with a linear search through the top K seen so
 far for each value in the input array, so its worst case is O(KN)
 comparisons.  Some of the sorting gurus on this list might have
 something to say about that but my take is that it seems fine for the
 tiny values of K and N that we're dealing with here, and it's nice
 that it doesn't need any space other than the output buffer, unlike
 some other top-K algorithms which would win for larger inputs.
>>
>> Yeah, it's improvement point.
>> But I'm assumed that the number of synchronous replication is not
>> large, so I use this algorithm as first version.
>> And I think that its worst case is O(K(N-K)). Am I missing something?
>
> You're right, I was dropping that detail, in the tradition of the
> hand-wavy school of big-O notation.  (I suppose you could skip the
> inner loop when the priority is lower than the current lowest
> priority, giving a O(N) best case when the walsenders are perfectly
> ordered by coincidence.  Probably a bad idea or just not worth
> worrying about.)

 Thank you for reviewing the patch.
 Yeah, I added the logic that skip the inner loop.

>
>> Attached latest version patch.
>
> +/*
> + * Obtain currently synced LSN location: write and flush, using priority
> - * In 9.1 we support only a single synchronous standby, chosen from a
> - * priority list of synchronous_standby_names. Before it can become the
> + * In 9.6 we support multiple synchronous standby, chosen from a priority
>
> s/standby/standbys/
>
> + * list of synchronous_standby_names. Before it can become the
>
> s/Before it can become the/Before any standby can become a/
>
>   * synchronous standby it must have caught up with the primary; that may
>   * take some time. Once caught up, the current highest priority standby
>
> s/standby/standbys/
>
>   * will release waiters from the queue.
>
> +bool
> +SyncRepGetSyncLsnsPriority(XLogRecPtr *write_pos, XLogRecPtr *flush_pos)
> +{
> + int sync_standbys[synchronous_standby_num];
>
> I think this should be sync_standbys[SYNC_REP_MAX_SYNC_STANDBY_NUM].
> (Variable sized arrays are a feature of C99 and PostgreSQL is written
> in C89.)
>
> +/*
> + * Populate a caller-supplied array which much have enough space for
> + * synchronous_standby_num. Returns position of standbys currently
> + * considered as synchronous, and its length.
> + */
> +int
> +SyncRepGetSyncStandbys(int *sync_standbys)
>
> s/much/must/ (my bad, in previous email).
>
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("The number of synchronous standbys must be smaller than the
> number of listed : %d",
> + synchronous_standby_num)));
>
> How about "the number of synchronous standbys exceeds the length of
> the standby list: %d"?  Error messages usually start with lower case,
> ':' is not usually preceded by a space.
>
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("The number of synchronous standbys must be between 1 and %d : 
> %d",

Re: [HACKERS] Several problems in tab-completions for SET/RESET

[Michael Paquier]

On Thu, Jan 28, 2016 at 9:32 PM, Fujii Masao  wrote:
> I found that the following tab-completions for SET/RESET which
> worked properly before doesn't work properly now in the master.
>
> 1. ALTER SYSTEM SET|RESET  lists nothing.
> 2. ALTER DATABASE xxx SET  lists nothing.
> 3. ALTER DATABASE xxx SET yyy  lists nothing.
> 4. ALTER DATABASE xxx SET datestyle TO  lists nothing.
>
> Attached patch fixes those problems.

-   else if (Matches4("ALTER", "SYSTEM", "SET|RESET", MatchAny))
+   else if (Matches3("ALTER", "SYSTEM", "SET|RESET"))
Good catch.

-   else if (Matches2("SET", MatchAny))
+   else if (TailMatches2("SET", MatchAny) &&
+!TailMatches4("UPDATE|DOMAIN", MatchAny,
MatchAny, MatchAny) &&
+!TailMatches1("TABLESPACE|SCHEMA") &&
+!ends_with(prev_wd, ')') &&
+!ends_with(prev_wd, '='))
COMPLETE_WITH_CONST("TO");
This gets... unreadable.

In order to maximize the amount of Matches() used, wouldn't it be
better to complete a bit more the list of completions directly in
ALTER DATABASE? This would make the code more readable.
-- 
Michael


-- 
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] Template for commit messages

[Bruce Momjian]

On Thu, Jan 28, 2016 at 11:30:58AM +0100, Tomas Vondra wrote:
> Any reason why not to adapt the git message conventions from kernel?
> 
> https://git.wiki.kernel.org/index.php/CommitMessageConventions
> 
> I'd expect there are tools already working with that format, making
> the life easier for us.

Good idea.  :-)  Updated template:

-- email subject limit -
-- gitweb summary limit --


Reported-by:

Bug:

Patch-by:

Reviewed-by:

Backpatch through:

I had to make up "Patch-by:" as it wasn't listed.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


-- 
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] Weighted Stats

[Alvaro Herrera]

I'm closing this for the current commitfest as returned-with-feedback.
Please resubmit for the 2016-03 CF once you have it.

Thanks!

-- 
Á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] Improvements of Hunspell dictionaries support

[Alvaro Herrera]

Artur Zakirov wrote:

> I undo the changes and the error will be raised. I will update the patch
> soon.

I don't think you ever did this.  I'm closing it now, but it sounds
useful stuff so please do resubmit for 2016-03.

-- 
Á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:Use PGDLLEXPORT for libpq

[Yury Zhuravlev]

Michael Paquier wrote:

Many companies use it, including mine, and likely EDB.

Ok, why? I wonder why it is?
--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres 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:Use PGDLLEXPORT for libpq

[Yury Zhuravlev]

So even if the main packages switched to
compiling with mingw, we'd probably still want to support MSVC.
MinGV more accessible. We can always say that after the release of X is not 
supported by the MSVC.

I do not propose to abandon the MSVC but its use to be justified.


Are we moving forward with the CMake stuff?  It would be *awesome* to
get rid of the MSVC build scripts, and perhaps we can move forward on
some smaller items such as PGDLLEXPORT markings as well.

Linux build done with installcheck tests. We testing this under CentOS7, 
Debian8, Gentoo, Ubuntu.

MSVC build 50% done. Yesterday made the work of the gendef.pl under CMake.

Current issues:
https://github.com/stalkerg/postgres_cmake/issues

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres 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] Improvements of Hunspell dictionaries support

[Artur Zakirov]

On 28.01.2016 14:19, Alvaro Herrera wrote:

Artur Zakirov wrote:


I undo the changes and the error will be raised. I will update the patch
soon.


I don't think you ever did this.  I'm closing it now, but it sounds
useful stuff so please do resubmit for 2016-03.



I'm working on the patch. I wanted to send this changes after all changes.

This version of the patch has a top-level comment. Another comments I 
will provides soon.


Also this patch has some changes with ternary operators.

> I don't think you ever did this.  I'm closing it now, but it sounds
> useful stuff so please do resubmit for 2016-03.

Moved to next CF.

--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
*** a/doc/src/sgml/textsearch.sgml
--- b/doc/src/sgml/textsearch.sgml
***
*** 2615,2632  SELECT plainto_tsquery('supernova star');
 
  
 
! To create an Ispell dictionary, use the built-in
! ispell template and specify several parameters:
 
! 
  
! CREATE TEXT SEARCH DICTIONARY english_ispell (
  TEMPLATE = ispell,
! DictFile = english,
! AffFile = english,
! StopWords = english
! );
  
  
 
  Here, DictFile, AffFile, and StopWords
--- 2615,2655 
 
  
 
! To create an Ispell dictionary perform these steps:
 
!
! 
!  
!   download dictionary configuration files. OpenOffice
!   extension files have the .oxt extension. It is necessary
!   to extract .aff and .dic files, change extensions
!   to .affix and .dict. For some dictionary
!   files it is also needed to convert characters to the UTF-8 encoding
!   with commands (for example, for norwegian language dictionary):
  
! iconv -f ISO_8859-1 -t UTF-8 -o nn_no.affix nn_NO.aff
! iconv -f ISO_8859-1 -t UTF-8 -o nn_no.dict nn_NO.dic
! 
!  
! 
! 
!  
!   copy files to the $SHAREDIR/tsearch_data directory
!  
! 
! 
!  
!   load files into PostgreSQL with the following command:
! 
! CREATE TEXT SEARCH DICTIONARY english_hunspell (
  TEMPLATE = ispell,
! DictFile = en_us,
! AffFile = en_us,
! Stopwords = english);
  
+  
+ 
+
  
 
  Here, DictFile, AffFile, and StopWords
***
*** 2643,2648  CREATE TEXT SEARCH DICTIONARY english_ispell (
--- 2666,2720 
 
  
 
+ The .affix file of Ispell has the following structure:
+ 
+ prefixes
+ flag *A:
+ .   >   RE  # As in enter > reenter
+ suffixes
+ flag T:
+ E   >   ST  # As in late > latest
+ [^AEIOU]Y   >   -Y,IEST # As in dirty > dirtiest
+ [AEIOU]Y>   EST # As in gray > grayest
+ [^EY]   >   EST # As in small > smallest
+ 
+
+
+ And the .dict file has the following structure:
+ 
+ lapse/ADGRS
+ lard/DGRS
+ large/PRTY
+ lark/MRS
+ 
+
+ 
+
+ Format of the .dict file is:
+ 
+ basic_form/affix_class_name
+ 
+
+ 
+
+ In the .affix file every affix flag is described in the
+ following format:
+ 
+ condition > [-stripping_letters,] adding_affix
+ 
+
+ 
+
+ Here, condition has a format similar to the format of regular expressions.
+ It can use groupings [...] and [^...].
+ For example, [AEIOU]Y means that the last letter of the word
+ is "y" and the penultimate letter is "a",
+ "e", "i", "o" or "u".
+ [^EY] means that the last letter is neither "e"
+ nor "y".
+
+ 
+
  Ispell dictionaries support splitting compound words;
  a useful feature.
  Notice that the affix file should specify a special flag using the
***
*** 2663,2668  SELECT ts_lexize('norwegian_ispell', 'sjokoladefabrikk');
--- 2735,2796 
  
 
  
+
+ MySpell is very similar to Hunspell.
+ The .affix file of Hunspell has the following structure:
+ 
+ PFX A Y 1
+ PFX A   0 re .
+ SFX T N 4
+ SFX T   0 st e
+ SFX T   y iest   [^aeiou]y
+ SFX T   0 est[aeiou]y
+ SFX T   0 est[^ey]
+ 
+
+ 
+
+ The first line of an affix class is the header. Fields of an affix rules are listed after the header:
+
+
+ 
+  
+   parameter name (PFX or SFX)
+  
+ 
+ 
+  
+   flag (name of the affix class)
+  
+ 
+ 
+  
+   stripping characters from beginning (at prefix) or end (at suffix) of the word
+  
+ 
+ 
+  
+   adding affix
+  
+ 
+ 
+  
+   condition that has a format similar to the format of regular expressions.
+  
+ 
+
+ 
+
+ The .dict file looks like the .dict file of
+ Ispell:
+ 
+ larder/M
+ lardy/RT
+ large/RSPMYT
+ largehearted
+ 
+
+ 
 
  
   MySpell does not support compound words.
*** a/src/backend/tsearch/spell.c
--- b/src/backend/tsearch/spell.c
***
*** 5,10 
--- 5,54 
   *
   * Portions Copyright (c) 1996-2016, PostgreSQL Global 

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

[Fujii Masao]

On Wed, Jan 20, 2016 at 2:35 PM, Masahiko Sawada  wrote:
> On Tue, Jan 19, 2016 at 1:52 AM, Thom Brown  wrote:
>> On 3 January 2016 at 13:26, Masahiko Sawada  wrote:
>>> On Fri, Dec 25, 2015 at 7:21 AM, Thomas Munro
>>>  wrote:
 On Fri, Dec 25, 2015 at 8:50 AM, Masahiko Sawada  
 wrote:
> On Wed, Dec 23, 2015 at 8:45 AM, Thomas Munro
>  wrote:
>> On Wed, Dec 23, 2015 at 3:50 PM, Thomas Munro
>>  wrote:
>>> If you got rid of SyncRepGetSyncStandbysOnePriority as suggested
>>> above, then this function could be renamed to SyncRepGetSyncStandbys.
>>> I think it would be a tiny bit nicer if it also took a Size n argument
>>> along with the output buffer pointer.
>
> Sorry, I could not get your point. SyncRepGetSyncStandbysPriority()
> function uses synchronous_standby_num which is global variable.
> But you mean that the number of synchronous standbys is given as
> function argument?

 Yeah, I was thinking of it as the output buffer size which I would be
 inclined to make more explicit (I am still coming to terms with the
 use of global variables in Postgres) but it doesn't matter, please
 disregard that suggestion.

>>> As for the body of that function (which I won't paste here), it
>>> contains an algorithm to find the top K elements in an array of N
>>> elements.  It does that with a linear search through the top K seen so
>>> far for each value in the input array, so its worst case is O(KN)
>>> comparisons.  Some of the sorting gurus on this list might have
>>> something to say about that but my take is that it seems fine for the
>>> tiny values of K and N that we're dealing with here, and it's nice
>>> that it doesn't need any space other than the output buffer, unlike
>>> some other top-K algorithms which would win for larger inputs.
>
> Yeah, it's improvement point.
> But I'm assumed that the number of synchronous replication is not
> large, so I use this algorithm as first version.
> And I think that its worst case is O(K(N-K)). Am I missing something?

 You're right, I was dropping that detail, in the tradition of the
 hand-wavy school of big-O notation.  (I suppose you could skip the
 inner loop when the priority is lower than the current lowest
 priority, giving a O(N) best case when the walsenders are perfectly
 ordered by coincidence.  Probably a bad idea or just not worth
 worrying about.)
>>>
>>> Thank you for reviewing the patch.
>>> Yeah, I added the logic that skip the inner loop.
>>>

> Attached latest version patch.

 +/*
 + * Obtain currently synced LSN location: write and flush, using priority
 - * In 9.1 we support only a single synchronous standby, chosen from a
 - * priority list of synchronous_standby_names. Before it can become the
 + * In 9.6 we support multiple synchronous standby, chosen from a priority

 s/standby/standbys/

 + * list of synchronous_standby_names. Before it can become the

 s/Before it can become the/Before any standby can become a/

   * synchronous standby it must have caught up with the primary; that may
   * take some time. Once caught up, the current highest priority standby

 s/standby/standbys/

   * will release waiters from the queue.

 +bool
 +SyncRepGetSyncLsnsPriority(XLogRecPtr *write_pos, XLogRecPtr *flush_pos)
 +{
 + int sync_standbys[synchronous_standby_num];

 I think this should be sync_standbys[SYNC_REP_MAX_SYNC_STANDBY_NUM].
 (Variable sized arrays are a feature of C99 and PostgreSQL is written
 in C89.)

 +/*
 + * Populate a caller-supplied array which much have enough space for
 + * synchronous_standby_num. Returns position of standbys currently
 + * considered as synchronous, and its length.
 + */
 +int
 +SyncRepGetSyncStandbys(int *sync_standbys)

 s/much/must/ (my bad, in previous email).

 + ereport(ERROR,
 + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 + errmsg("The number of synchronous standbys must be smaller than the
 number of listed : %d",
 + synchronous_standby_num)));

 How about "the number of synchronous standbys exceeds the length of
 the standby list: %d"?  Error messages usually start with lower case,
 ':' is not usually preceded by a space.

 + ereport(ERROR,
 + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 + errmsg("The number of synchronous standbys must be between 1 and %d : 
 %d",

 s/The/the/, s/ : /: /
>>>
>>> Fixed you mentioned.
>>>
>>> Attached latest v5 patch.
>>> Please review it.
>>
>> synchronous_standby_num doesn't appear to be a valid GUC name:
>>

Re: [HACKERS] Template for commit messages

[Tomas Vondra]

Dne 28. 1. 2016 11:57 napsal uživatel "Alvaro Herrera" <
alvhe...@2ndquadrant.com>:
>
> Magnus Hagander wrote:
>
> > They also had tested-by, it might be an idea to include that as well?
>
> How about
> User-Interface-Bikeshedded-By:
> ?

+1

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

Re: [HACKERS] Proposal:Use PGDLLEXPORT for libpq

[Yury Zhuravlev]

Craig Ringer wrote:

Yeah, strongly agree there.

CMake has excellent MSVC support btw.


Yes... but I found only hack way for call gendef.pl in pre_link stage (get 
objects folder name). CMake suggests that we use for normal MSVC ways to 
create dll.



OTOH MinGW relies on reverse-engineered headers, an old gcc fork, and has had 
some pretty nasty bugs.


You about MinGW or MinGW64? 
--

Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres 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] Template for commit messages

[Bruce Momjian]

On Thu, Jan 28, 2016 at 03:52:00AM -0500, Bruce Momjian wrote:
> There has been a request in the FOSDEM developers meeting that
> committers use a more consistent format for commit messages.  This is
> the format I use:

Here is an updated version that includes a potential bug number:

-- email subject limit -
-- gitweb summary limit --


Report by

Bug #

Patch by

Reviewed by

Backpatch through

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


-- 
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] Template for commit messages

[Tomas Vondra]

Hi,

On 01/28/2016 11:06 AM, Bruce Momjian wrote:

On Thu, Jan 28, 2016 at 03:52:00AM -0500, Bruce Momjian wrote:

There has been a request in the FOSDEM developers meeting that
committers use a more consistent format for commit messages.  This is
the format I use:


Here is an updated version that includes a potential bug number:

-- email subject limit -
-- gitweb summary limit --


Report by

Bug #

Patch by

Reviewed by

Backpatch through



Any reason why not to adapt the git message conventions from kernel?

https://git.wiki.kernel.org/index.php/CommitMessageConventions

I'd expect there are tools already working with that format, making the 
life easier for us.


regards

--
Tomas Vondra  http://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] VACUUM Progress Checker.

[Rahila Syed]

>+if(!scan_all)
>+scanned_heap_pages = scanned_heap_pages +
>next_not_all_visible_block;

>I don't want to be too much of a stickler for details here, but it
>seems to me that this is an outright lie.

Initially the scanned_heap_pages were meant to report just the scanned
pages and skipped pages were not added to the count.  Instead the skipped
pages were deduced from number of total heap pages to be scanned to make
the number of scanned pages eventually add up to total heap pages.   As per
comments received later total heap pages were kept constant and skipped
pages count was added to scanned pages to make the count add up to total
heap pages at the end of scan. That said, as suggested, scanned_heap_pages
should be renamed to current_heap_page to report current blkno in
lazy_scan_heap loop which will add up to total heap pages(nblocks) at the
end of scan. And scanned_heap_pages can be reported as a separate number
which wont contain skipped pages.


>+/*
>+ * Reporting vacuum progress to statistics collector
>+ */

>This patch doesn't report anything to the statistics collector, nor should
it.
Yes. This was incorrectly added initially by referring to similar
pgstat_report interface functions.

Thank you,
Rahila Syed

On Thu, Jan 28, 2016 at 2:27 AM, Robert Haas  wrote:

> On Tue, Jan 26, 2016 at 11:37 PM, Vinayak Pokale 
> wrote:
> > Hi,
> >
> > Please find attached updated patch with an updated interface.
>
> Well, this isn't right.  You've got this sort of thing:
>
> +scanned_index_pages += RelationGetNumberOfBlocks(Irel[i]);
> +/* Report progress to the statistics collector */
> +pgstat_report_progress_update_message(0, progress_message);
> +pgstat_report_progress_update_counter(1, scanned_heap_pages);
> +pgstat_report_progress_update_counter(3, scanned_index_pages);
> +pgstat_report_progress_update_counter(4,
> vacrelstats->num_index_scans + 1);
>
> The point of having pgstat_report_progress_update_counter() is so that
> you can efficiently update a single counter without having to update
> everything, when only one counter has changed.  But here you are
> calling this function a whole bunch of times in a row, which
> completely misses the point - if you are updating all the counters,
> it's more efficient to use an interface that does them all at once
> instead of one at a time.
>
> But there's a second problem here, too, which is that I think you've
> got this code in the wrong place.  The second point of the
> pgstat_report_progress_update_counter interface is that this should be
> cheap enough that we can do it every time the counter changes.  That's
> not what you are doing here.  You're updating the counters at various
> points in the code and just pushing new values for all of them
> regardless of which ones have changed.  I think you should find a way
> that you can update the value immediately at the exact moment it
> changes.  If that seems like too much of a performance hit we can talk
> about it, but I think the value of this feature will be greatly
> weakened if users can't count on it to be fully and continuously up to
> the moment.  When something gets stuck, you want to know where it's
> stuck, not approximately kinda where it's stuck.
>
> +if(!scan_all)
> +scanned_heap_pages = scanned_heap_pages +
> next_not_all_visible_block;
>
> I don't want to be too much of a stickler for details here, but it
> seems to me that this is an outright lie.  The number of scanned pages
> does not go up when we decide to skip some pages, because scanning and
> skipping aren't the same thing.  If we're only going to report one
> number here, it needs to be called something like "current heap page",
> and then you can just report blkno at the top of each iteration of
> lazy_scan_heap's main loop.  If you want to report the numbers of
> scanned and skipped pages separately that'd be OK too, but you can't
> call it the number of scanned pages and then actually report a value
> that is not that.
>
> +/*
> + * Reporting vacuum progress to statistics collector
> + */
>
> This patch doesn't report anything to the statistics collector, nor should
> it.
>
> Instead of making the SQL-visible function
> pg_stat_get_vacuum_progress(), I think it should be something more
> generic like pg_stat_get_command_progress().  Maybe VACUUM will be the
> only command that reports into that feature for right now, but I'd
> hope for us to change that pretty soon after we get the first patch
> committed.
>
> --
> 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] Template for commit messages

[Alvaro Herrera]

Magnus Hagander wrote:

> They also had tested-by, it might be an idea to include that as well?

How about
User-Interface-Bikeshedded-By:
?

-- 
Á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] Template for commit messages

[Alvaro Herrera]

Bruce Momjian wrote:
> There has been a request in the FOSDEM developers meeting that
> committers use a more consistent format for commit messages.

Let me point out that the reason this is being put forward is to make
sure we have the reviewers listed for each patch, so that we can add a
paragraph somewhere at the bottom of the release notes listing people
that helped with the release.

-- 
Á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] dblink: add polymorphic functions.

[Alvaro Herrera]

Joe Conway wrote:

> Ok, back to the drawing board. Thanks for the feedback.

Closing this one as returned-with-feedback.  Please do resubmit for
CF 2016-03.

-- 
Á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] POC, WIP: OR-clause support for indexes

[Alvaro Herrera]

I think this is very exciting stuff, but since you didn't submit an
updated patch after David's review, I'm closing it for now as
returned-with-feedback.  Please submit a new version once you have it.

-- 
Á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] Template for commit messages

[Magnus Hagander]

On Thu, Jan 28, 2016 at 11:49 AM, Bruce Momjian  wrote:

> On Thu, Jan 28, 2016 at 11:30:58AM +0100, Tomas Vondra wrote:
> > Any reason why not to adapt the git message conventions from kernel?
> >
> > https://git.wiki.kernel.org/index.php/CommitMessageConventions
> >
> > I'd expect there are tools already working with that format, making
> > the life easier for us.
>
> Good idea.  :-)  Updated template:
>
> -- email subject limit -
> -- gitweb summary limit --
>
>
> Reported-by:
>
> Bug:
>
> Patch-by:
>
> Reviewed-by:
>
> Backpatch through:
>
> I had to make up "Patch-by:" as it wasn't listed.
>
>
The original format uses the author for that (author != committer in git),
but that changes how we work a bit more. I'd be happy to just clal it
"Author" rather than "Patch-by".

I also suggest a - in "Backpatch-through:" -- since all the others are
intentionally mad ewithout a space, I assume that's for easier parsing.

They also had tested-by, it might be an idea to include that as well?


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Relation extension scalability

[Dilip Kumar]

On Thu, Jan 28, 2016 at 4:53 PM, Dilip Kumar  wrote:

> I did not find in regression in normal case.
> Note: I tested it with previous patch extend_num_pages=10 (guc parameter)
> so that we can see any impact on overall system.
>

Just forgot to mentioned That i have run pgbench read-write case.

S.F: 300

./pgbench -j $ -c $ -T 1800 -M Prepared postgres

Tested with 1,8,16,32,64 Threads and did not see any regression with patch.


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Template for commit messages

[Bruce Momjian]

On Thu, Jan 28, 2016 at 11:53:44AM +0100, Magnus Hagander wrote:
> The original format uses the author for that (author != committer in git), but
> that changes how we work a bit more. I'd be happy to just clal it "Author"
> rather than "Patch-by".
> 
> I also suggest a - in "Backpatch-through:" -- since all the others are
> intentionally mad ewithout a space, I assume that's for easier parsing.
> 
> They also had tested-by, it might be an idea to include that as well? 

OK, updated based on those suggestions:

-- email subject limit -
-- gitweb summary limit --


Reported-by:

Bug:

Author:

Reviewed-by:

Tested-by:

Backpatch-through:

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


-- 
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] Relation extension scalability

[Dilip Kumar]

On Mon, Jan 25, 2016 at 11:59 AM, Dilip Kumar  wrote:

1.
>> Patch is not getting compiled.
>>
>> 1>src/backend/access/heap/hio.c(480): error C2065: 'buf' : undeclared
>> identifier
>>
> Oh, My mistake, my preprocessor is ignoring this error and relacing it
> with BLKSIZE
>

FIXED


> 3. I think you don't need to multi-extend a relation if
>> HEAP_INSERT_SKIP_FSM is used, as for that case it anyways try to
>> get a new page by extending a relation.
>>
>
> So i will change this..
>

FIXED

>
> 4. Again why do you need this multi-extend optimization for local
>> relations (those only accessible to current backend)?
>>
>
> I think we can change this while adding the  table level
> "extend_by_blocks" for local table we will not allow this property, so no
> need to change at this place.
> What do you think ?
>

Now I have added table level parameter for specifying the number of blocks,
So do you think that we still need to block it, as user can control it,
Moreover i think if user want to use for local table then no harm in it at
least by extending in one shot he avoid multiple call of Extension lock,
though there will be no contention.

What is your opinion ?


5. Do we need this for nbtree as well?  One way to check that
>> is by Copying large data in table having index.
>>
>> Ok, i will try this test and update.
>

I tried to load data to table with Index and tried to analyze bottleneck
using perf, and found btcompare was taking maximum time, still i don't deny
that it can not get benefited by multi extend.

So i tried quick POC for this, but i realize that even though we extend
multiple page for index and add to FSM, it will be updated only in current
page, Information about new free page will be propagated to root page only
during vacuum, And unlike heap Btree always search FSM from root and it
will not find the extra added pages.

So i think we can analyze this topic separately for index multi extend and
find is there are cases where index multi extend can give benefit.

Note: Test with both data and WAL on Magnetic Disk : No significant
>>> improvement visible
>>> -- I think wall write is becoming bottleneck in this case.
>>>
>>>
>> In that case, can you try the same test with un-logged tables?
>>
> Date with un-logged table

Test Init:

./psql -d postgres -c "COPY (select g.i::text FROM generate_series(1,
1) g(i)) TO '/tmp/copybinary' WITH BINARY";
echo COPY data from '/tmp/copybinary' WITH BINARY; > copy_script
./psql -d postgres -c "create unlogged table data (data text)" --> base
./psql -d postgres -c "create unlogged table data (data text)
with(extend_by_blocks=50)" --patch

test_script:

./psql -d postgres -c "truncate table data"
./psql -d postgres -c "checkpoint"
./pgbench -f copy_script -T 120 -c$ -j$ postgres

Shared Buffer48GB
Table:Unlogged Table
ench -c$ -j$ -f -M Prepared postgres

ClientsBasepatch
1178   180
2337   338
4265   601
8167   805

Also, it is good to check the performance of patch with read-write work
>> load to ensure that extending relation in multiple-chunks should not
>> regress such cases.
>>
>
> Ok
>

I did not find in regression in normal case.
Note: I tested it with previous patch extend_num_pages=10 (guc parameter)
so that we can see any impact on overall system.


> Currently i have kept extend_num_page as session level parameter but i
>>> think later we can make this as table property.
>>> Any suggestion on this ?
>>>
>>>
>> I think we should have a new storage_parameter at table level
>> extend_by_blocks or something like that instead of GUC. The
>> default value of this parameter should be 1 which means retain
>> current behaviour by default.
>>
> +1
>

Changed it to table level storage parameter. I kept max_limit to 100 any
suggestion on this ? should we increase it ?

latest patch is attached..

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 86b9ae1..76f9a21 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -268,6 +268,16 @@ static relopt_int intRelOpts[] =
 #endif
 	},
 
+	{
+		{
+			"extend_by_blocks",
+			"Number of blocks to be added to relation in every extend call",
+			RELOPT_KIND_HEAP,
+			AccessExclusiveLock
+		},
+		1, 1, 100
+	},
+
 	/* list terminator */
 	{{NULL}}
 };
@@ -1291,7 +1301,9 @@ default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
 		{"autovacuum_analyze_scale_factor", RELOPT_TYPE_REAL,
 		offsetof(StdRdOptions, autovacuum) +offsetof(AutoVacOpts, analyze_scale_factor)},
 		{"user_catalog_table", RELOPT_TYPE_BOOL,
-		offsetof(StdRdOptions, user_catalog_table)}
+		offsetof(StdRdOptions, user_catalog_table)},
+		{"extend_by_blocks", RELOPT_TYPE_INT,
+		offsetof(StdRdOptions, extend_by_blocks)}
 	};
 
 	options = 

Re: [HACKERS] Proposal:Use PGDLLEXPORT for libpq

[Yury Zhuravlev]

Craig Ringer wrote:
I strongly disagree. MSVC is a high quality compiler and the 
primary tool for the platform.
Ok. And we not suport MSVC2015 now. Either we support the platform normally 
or throwing it. 
Now it all looks like a zombie.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres 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] Improvements of Hunspell dictionaries support

[Artur Zakirov]

Sorry, I don't know why this thread was moved to another thread.

I duplicate the patch here.


On 28.01.2016 14:19, Alvaro Herrera wrote:

Artur Zakirov wrote:


I undo the changes and the error will be raised. I will update the patch
soon.


I don't think you ever did this. I'm closing it now, but it sounds
useful stuff so please do resubmit for 2016-03.



I'm working on the patch. I wanted to send this changes after all changes.

This version of the patch has a top-level comment. Another comments I will 
provides soon.

Also this patch has some changes with ternary operators.


I don't think you ever did this. I'm closing it now, but it sounds
useful stuff so please do resubmit for 2016-03.


Moved to next CF.




--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
*** a/doc/src/sgml/textsearch.sgml
--- b/doc/src/sgml/textsearch.sgml
***
*** 2615,2632  SELECT plainto_tsquery('supernova star');
 
  
 
! To create an Ispell dictionary, use the built-in
! ispell template and specify several parameters:
 
! 
  
! CREATE TEXT SEARCH DICTIONARY english_ispell (
  TEMPLATE = ispell,
! DictFile = english,
! AffFile = english,
! StopWords = english
! );
  
  
 
  Here, DictFile, AffFile, and StopWords
--- 2615,2655 
 
  
 
! To create an Ispell dictionary perform these steps:
 
!
! 
!  
!   download dictionary configuration files. OpenOffice
!   extension files have the .oxt extension. It is necessary
!   to extract .aff and .dic files, change extensions
!   to .affix and .dict. For some dictionary
!   files it is also needed to convert characters to the UTF-8 encoding
!   with commands (for example, for norwegian language dictionary):
  
! iconv -f ISO_8859-1 -t UTF-8 -o nn_no.affix nn_NO.aff
! iconv -f ISO_8859-1 -t UTF-8 -o nn_no.dict nn_NO.dic
! 
!  
! 
! 
!  
!   copy files to the $SHAREDIR/tsearch_data directory
!  
! 
! 
!  
!   load files into PostgreSQL with the following command:
! 
! CREATE TEXT SEARCH DICTIONARY english_hunspell (
  TEMPLATE = ispell,
! DictFile = en_us,
! AffFile = en_us,
! Stopwords = english);
  
+  
+ 
+
  
 
  Here, DictFile, AffFile, and StopWords
***
*** 2643,2648  CREATE TEXT SEARCH DICTIONARY english_ispell (
--- 2666,2720 
 
  
 
+ The .affix file of Ispell has the following structure:
+ 
+ prefixes
+ flag *A:
+ .   >   RE  # As in enter > reenter
+ suffixes
+ flag T:
+ E   >   ST  # As in late > latest
+ [^AEIOU]Y   >   -Y,IEST # As in dirty > dirtiest
+ [AEIOU]Y>   EST # As in gray > grayest
+ [^EY]   >   EST # As in small > smallest
+ 
+
+
+ And the .dict file has the following structure:
+ 
+ lapse/ADGRS
+ lard/DGRS
+ large/PRTY
+ lark/MRS
+ 
+
+ 
+
+ Format of the .dict file is:
+ 
+ basic_form/affix_class_name
+ 
+
+ 
+
+ In the .affix file every affix flag is described in the
+ following format:
+ 
+ condition > [-stripping_letters,] adding_affix
+ 
+
+ 
+
+ Here, condition has a format similar to the format of regular expressions.
+ It can use groupings [...] and [^...].
+ For example, [AEIOU]Y means that the last letter of the word
+ is "y" and the penultimate letter is "a",
+ "e", "i", "o" or "u".
+ [^EY] means that the last letter is neither "e"
+ nor "y".
+
+ 
+
  Ispell dictionaries support splitting compound words;
  a useful feature.
  Notice that the affix file should specify a special flag using the
***
*** 2663,2668  SELECT ts_lexize('norwegian_ispell', 'sjokoladefabrikk');
--- 2735,2796 
  
 
  
+
+ MySpell is very similar to Hunspell.
+ The .affix file of Hunspell has the following structure:
+ 
+ PFX A Y 1
+ PFX A   0 re .
+ SFX T N 4
+ SFX T   0 st e
+ SFX T   y iest   [^aeiou]y
+ SFX T   0 est[aeiou]y
+ SFX T   0 est[^ey]
+ 
+
+ 
+
+ The first line of an affix class is the header. Fields of an affix rules are listed after the header:
+
+
+ 
+  
+   parameter name (PFX or SFX)
+  
+ 
+ 
+  
+   flag (name of the affix class)
+  
+ 
+ 
+  
+   stripping characters from beginning (at prefix) or end (at suffix) of the word
+  
+ 
+ 
+  
+   adding affix
+  
+ 
+ 
+  
+   condition that has a format similar to the format of regular expressions.
+  
+ 
+
+ 
+
+ The .dict file looks like the .dict file of
+ Ispell:
+ 
+ larder/M
+ lardy/RT
+ large/RSPMYT
+ largehearted
+ 
+
+ 
 
  
   MySpell does not support compound words.
*** a/src/backend/tsearch/spell.c
--- b/src/backend/tsearch/spell.c

Re: [HACKERS] Several problems in tab-completions for SET/RESET

[Fujii Masao]

On Thu, Jan 28, 2016 at 10:50 PM, Masahiko Sawada  wrote:
> On Thu, Jan 28, 2016 at 10:15 PM, Michael Paquier
>  wrote:
>> On Thu, Jan 28, 2016 at 9:32 PM, Fujii Masao  wrote:
>>> I found that the following tab-completions for SET/RESET which
>>> worked properly before doesn't work properly now in the master.
>>>
>>> 1. ALTER SYSTEM SET|RESET  lists nothing.
>>> 2. ALTER DATABASE xxx SET  lists nothing.
>>> 3. ALTER DATABASE xxx SET yyy  lists nothing.
>>> 4. ALTER DATABASE xxx SET datestyle TO  lists nothing.
>>>
>>> Attached patch fixes those problems.
>>
>> -   else if (Matches4("ALTER", "SYSTEM", "SET|RESET", MatchAny))
>> +   else if (Matches3("ALTER", "SYSTEM", "SET|RESET"))
>> Good catch.
>>
>> -   else if (Matches2("SET", MatchAny))
>> +   else if (TailMatches2("SET", MatchAny) &&
>> +!TailMatches4("UPDATE|DOMAIN", MatchAny,
>> MatchAny, MatchAny) &&
>> +!TailMatches1("TABLESPACE|SCHEMA") &&
>> +!ends_with(prev_wd, ')') &&
>> +!ends_with(prev_wd, '='))
>> COMPLETE_WITH_CONST("TO");
>
> This change breaks tab completion for ALTER TABLE ... SET
> [WITH/LOGGED/UNLOGGED].

Thanks for the review!
Fixed.

> It think it should be
>> +   else if (Matches2("SET", MatchAny) &&
>
> Related to it, I found tab completion for ALTER TABLE .. SET WITH,
> which doesn't working well.
> Patch is attached.

Please see the latest patch that I posted upthread.
I included your patch in that patch.

Regards,

-- 
Fujii Masao


-- 
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] Several problems in tab-completions for SET/RESET

[Fujii Masao]

On Thu, Jan 28, 2016 at 10:15 PM, Michael Paquier
 wrote:
> On Thu, Jan 28, 2016 at 9:32 PM, Fujii Masao  wrote:
>> I found that the following tab-completions for SET/RESET which
>> worked properly before doesn't work properly now in the master.
>>
>> 1. ALTER SYSTEM SET|RESET  lists nothing.
>> 2. ALTER DATABASE xxx SET  lists nothing.
>> 3. ALTER DATABASE xxx SET yyy  lists nothing.
>> 4. ALTER DATABASE xxx SET datestyle TO  lists nothing.
>>
>> Attached patch fixes those problems.
>
> -   else if (Matches4("ALTER", "SYSTEM", "SET|RESET", MatchAny))
> +   else if (Matches3("ALTER", "SYSTEM", "SET|RESET"))
> Good catch.
>
> -   else if (Matches2("SET", MatchAny))
> +   else if (TailMatches2("SET", MatchAny) &&
> +!TailMatches4("UPDATE|DOMAIN", MatchAny,
> MatchAny, MatchAny) &&
> +!TailMatches1("TABLESPACE|SCHEMA") &&
> +!ends_with(prev_wd, ')') &&
> +!ends_with(prev_wd, '='))
> COMPLETE_WITH_CONST("TO");
> This gets... unreadable.
>
> In order to maximize the amount of Matches() used, wouldn't it be
> better to complete a bit more the list of completions directly in
> ALTER DATABASE? This would make the code more readable.

Thanks for the review!

I removed the above and added the following for that case.

+/* Complete ALTER DATABASE|FUNCTION|ROLE|USER ... SET  */
+else if (Matches2("ALTER", "DATABASE|FUNCTION|ROLE|USER") &&
+ TailMatches2("SET", MatchAny))
+COMPLETE_WITH_LIST2("FROM CURRENT", "TO");

Attached is the updated version of the patch.
I also added the change that Sawada suggested, to the patch.

Regards,

-- 
Fujii Masao
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
***
*** 1553,1559  psql_completion(const char *text, int start, int end)
  	else if (Matches2("ALTER", "SYSTEM"))
  		COMPLETE_WITH_LIST2("SET", "RESET");
  	/* ALTER SYSTEM SET|RESET  */
! 	else if (Matches4("ALTER", "SYSTEM", "SET|RESET", MatchAny))
  		COMPLETE_WITH_QUERY(Query_for_list_of_alter_system_set_vars);
  	/* ALTER VIEW  */
  	else if (Matches3("ALTER", "VIEW", MatchAny))
--- 1553,1559 
  	else if (Matches2("ALTER", "SYSTEM"))
  		COMPLETE_WITH_LIST2("SET", "RESET");
  	/* ALTER SYSTEM SET|RESET  */
! 	else if (Matches3("ALTER", "SYSTEM", "SET|RESET"))
  		COMPLETE_WITH_QUERY(Query_for_list_of_alter_system_set_vars);
  	/* ALTER VIEW  */
  	else if (Matches3("ALTER", "VIEW", MatchAny))
***
*** 1754,1759  psql_completion(const char *text, int start, int end)
--- 1754,1762 
  	 */
  	else if (Matches5("ALTER", "TABLE", MatchAny, "SET", "TABLESPACE"))
  		COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces);
+ 	/* If we have ALTER TABLE  SET WITH provide OIDS */
+ 	else if (Matches5("ALTER", "TABLE", MatchAny, "SET", "WITH"))
+ 		COMPLETE_WITH_CONST("OIDS");
  	/* If we have ALTER TABLE  SET WITHOUT provide CLUSTER or OIDS */
  	else if (Matches5("ALTER", "TABLE", MatchAny, "SET", "WITHOUT"))
  		COMPLETE_WITH_LIST2("CLUSTER", "OIDS");
***
*** 2702,2708  psql_completion(const char *text, int start, int end)
  
  /* SET, RESET, SHOW */
  	/* Complete with a variable name */
! 	else if (Matches1("SET|RESET"))
  		COMPLETE_WITH_QUERY(Query_for_list_of_set_vars);
  	else if (Matches1("SHOW"))
  		COMPLETE_WITH_QUERY(Query_for_list_of_show_vars);
--- 2705,2711 
  
  /* SET, RESET, SHOW */
  	/* Complete with a variable name */
! 	else if (TailMatches1("SET|RESET") && !TailMatches3("UPDATE", MatchAny, "SET"))
  		COMPLETE_WITH_QUERY(Query_for_list_of_set_vars);
  	else if (Matches1("SHOW"))
  		COMPLETE_WITH_QUERY(Query_for_list_of_show_vars);
***
*** 2743,2750  psql_completion(const char *text, int start, int end)
  	/* Complete SET  with "TO" */
  	else if (Matches2("SET", MatchAny))
  		COMPLETE_WITH_CONST("TO");
  	/* Suggest possible variable values */
! 	else if (Matches3("SET", MatchAny, "TO|="))
  	{
  		/* special cased code for individual GUCs */
  		if (TailMatches2("DateStyle", "TO|="))
--- 2746,2757 
  	/* Complete SET  with "TO" */
  	else if (Matches2("SET", MatchAny))
  		COMPLETE_WITH_CONST("TO");
+ 	/* Complete ALTER DATABASE|FUNCTION|ROLE|USER ... SET  */
+ 	else if (Matches2("ALTER", "DATABASE|FUNCTION|ROLE|USER") &&
+ 			 TailMatches2("SET", MatchAny))
+ 		COMPLETE_WITH_LIST2("FROM CURRENT", "TO");
  	/* Suggest possible variable values */
! 	else if (TailMatches3("SET", MatchAny, "TO|="))
  	{
  		/* special cased code for individual GUCs */
  		if (TailMatches2("DateStyle", "TO|="))

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

Re: CustomScan in a larger structure (RE: [HACKERS] CustomScan support on readfuncs.c)

[Kouhei Kaigai]

> On Mon, Jan 25, 2016 at 8:06 PM, Kouhei Kaigai  wrote:
> > Sorry for my late response. I've been unavailable to have enough
> > time to touch code for the last 1.5 month.
> >
> > The attached patch is a revised one to handle private data of
> > foregn/custom scan node more gracefully.
> >
> > The overall consensus upthread were:
> > - A new ExtensibleNodeMethods structure defines a unique name
> >   and a set of callbacks to handle node copy, serialization,
> >   deserialization and equality checks.
> > - (Foreign|Custom)(Path|Scan|ScanState) are first host of the
> >   ExtensibleNodeMethods, to allow extension to define larger
> >   structure to store its private fields.
> > - ExtensibleNodeMethods does not support variable length
> >   structure (like a structure with an array on the tail, use
> >   separately allocated array).
> > - ExtensibleNodeMethods shall be registered on _PG_init() of
> >   extensions.
> >
> > The 'pgsql-v9.6-custom-private.v3.patch' is the main part of
> > this feature. As I pointed out before, it uses dynhash instead
> > of the self invented hash table.
> 
> On a first read-through, I see nothing in this patch to which I would
> want to object except for the fact that the comments and documentation
> need some work from a native speaker of English.  It looks like what
> we discussed, and I think it's an improvement over what we have now.
>
Thanks,

Do you think we shall allow to register same extensible node name for
different node types? Like, "GpuJoin" for any of CustomPath, CustomScan
and CustomScanState. Or, do we avoid this using different name for each?

--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


-- 
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] Several problems in tab-completions for SET/RESET

[Michael Paquier]

On Fri, Jan 29, 2016 at 11:53 AM, Fujii Masao  wrote:
> I removed the above and added the following for that case.
>
> +/* Complete ALTER DATABASE|FUNCTION|ROLE|USER ... SET  */
> +else if (Matches2("ALTER", "DATABASE|FUNCTION|ROLE|USER") &&
> + TailMatches2("SET", MatchAny))
> +COMPLETE_WITH_LIST2("FROM CURRENT", "TO");
>
> Attached is the updated version of the patch.

"ALTER FUNCTION foo(bar)" suggests OWNER TO, RENAME TO and SET SCHEMA.
I think that we had better suggesting SET instead of SET SCHEMA, and
add SCHEMA in the list of things suggested by SET.

"ALTER DATABASE foodb SET foo_param" should suggest TO/= but that's
not the case. After adding TO/= manually, a list of values is
suggested though. Same problem with ALTER ROLE and ALTER FUNCTION.

> I also added the change that Sawada suggested, to the patch.

OK.
-- 
Michael


-- 
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] postgres_fdw join pushdown (was Re: Custom/Foreign-Join-APIs)

[Robert Haas]

On Thu, Jan 28, 2016 at 11:26 AM, Ashutosh Bapat
 wrote:
> 2. pg_fdw_join_v3.patch: changes to postgres_fdw - more description below

This patch no longer quite applies because of conflicts with one of
your other patches that I applied today (cf. commit
fbe5a3fb73102c2cfec114a67943f4474383).

And then I broke it some more by committing a patch to extract
deparseLockingClause from postgresGetForeignPlan and move it to
deparse.c, but that should pretty directly reduce the size of this
patch.  I wonder if there are any other bits of refactoring of that
sort that we can do in advance of landing the main patch, just to
simplify review.  But I'm not sure there are: this patch removes very
little existing code; it just adds a ton of new stuff.

I think the names deparseColumnRefForJoinrel and
deparseColumnRefForBaserel are better than the previous names, but I
would capitalize the last "r", so "Rel" instead of "rel".  But it's
weird that we have those functions and also just plain old
deparseColumnRef, which is logically part of
deparseColumnRefForBaserel but inexplicably remains a separate
function.  I still don't see why you can't just add a bunch of new
logic to the existing deparseColumnRef, change the last argument to be
of type deparse_expr_cxt instead of PlannerInfo, and have that one
function simply handle more cases than it does currently.

It seems unlikely to me that postgresGetForeignPlan really needs to
call list_free_deep(fdw_scan_tlist).  Can't we just let memory context
reset clean that up?

In postgresGetForeignPlan (and I think some other functions), you
renamed the argument from baserel to foreignrel.  But I think it would
be better to just go with "rel".  We do that elsewhere in various
places, and it seems fine here too.  And it's shorter.

copy_path_for_epq_recheck() and friends are really ugly.  Should we
consider just adding copyObject() support for those node types
instead?

The error message quality in conversion_error_callback() looks
unacceptably poor.  The column number we're proposing to output will
be utterly meaningless to the user.  It would ideally be desirable to
output the base table name and the column name from that table.

I'm sure there's more -- this is a huge patch and I don't fully
understand it yet -- but I'm out of energy for tonight.

-- 
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: CustomScan in a larger structure (RE: [HACKERS] CustomScan support on readfuncs.c)

[Robert Haas]

On Thu, Jan 28, 2016 at 10:18 PM, Kouhei Kaigai  wrote:
> Do you think we shall allow to register same extensible node name for
> different node types? Like, "GpuJoin" for any of CustomPath, CustomScan
> and CustomScanState. Or, do we avoid this using different name for each?

I'd say a different name for each.  That's our current convention, and
I don't see much reason to change it.

-- 
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: CustomScan in a larger structure (RE: [HACKERS] CustomScan support on readfuncs.c)

[Kouhei Kaigai]

> On Thu, Jan 28, 2016 at 10:18 PM, Kouhei Kaigai  wrote:
> > Do you think we shall allow to register same extensible node name for
> > different node types? Like, "GpuJoin" for any of CustomPath, CustomScan
> > and CustomScanState. Or, do we avoid this using different name for each?
> 
> I'd say a different name for each.  That's our current convention, and
> I don't see much reason to change it.
>
OK, it is not a serious problem, at least, for my use cases.
A convention like "GpuJoinPath", "GpuJoin" and "GpuJoinState" are sufficient.
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


-- 
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] Template for commit messages

[Bruce Momjian]

On Thu, Jan 28, 2016 at 09:31:46AM -0500, Robert Haas wrote:
> On Thu, Jan 28, 2016 at 6:16 AM, Tomas Vondra
>  wrote:
> >> How about
> >> User-Interface-Bikeshedded-By:
> >> ?
> >
> > +1
> 
> That's sort of implicitly pejorative.   Maybe we could find another
> way to phrase that, like "Design-suggestions-by" or maybe a more
> general "Suggestions-by".

I wasn't sure if "User-Interface-Bikeshedded-By" related to the patch,
or to the discussion of labels for the commit messages.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


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

[HACKERS] New committer

[Magnus Hagander]

Hello!

The PostgreSQL core team would like to welcome Dean Rasheed as a new
committer for the PostgreSQL project.

Dean - welcome! Now let's see how quickly you can break the buildfarm!

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Template for commit messages

[Robert Haas]

On Thu, Jan 28, 2016 at 9:34 AM, Bruce Momjian  wrote:
> On Thu, Jan 28, 2016 at 09:31:46AM -0500, Robert Haas wrote:
>> On Thu, Jan 28, 2016 at 6:16 AM, Tomas Vondra
>>  wrote:
>> >> How about
>> >> User-Interface-Bikeshedded-By:
>> >> ?
>> >
>> > +1
>>
>> That's sort of implicitly pejorative.   Maybe we could find another
>> way to phrase that, like "Design-suggestions-by" or maybe a more
>> general "Suggestions-by".
>
> I wasn't sure if "User-Interface-Bikeshedded-By" related to the patch,
> or to the discussion of labels for the commit messages.

I thought it was a proposed commit message label and that the original
suggestion was tongue-in-cheek, but I might be misreading things.

-- 
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] Mac OS: invalid byte sequence for encoding "UTF8"

[Artur Zakirov]

On 27.01.2016 15:28, Artur Zakirov wrote:

On 27.01.2016 14:14, Stas Kelvich wrote:

Hi.

I tried that and confirm strange behaviour. It seems that problem with
small cyrillic letter ‘х’. (simplest obscene language filter? =)

That can be reproduced with simpler test

Stas




The test program was corrected. Now it uses wchar_t type. And it works
correctly and gives right output.

I think the NIImportOOAffixes() in spell.c should be corrected to avoid
this bug.



I have attached a patch. It adds new functions parse_ooaffentry() and 
get_nextentry() and fixes a couple comments.


Now russian and other supported dictionaries can be used for text search 
in Mac OS.


parse_ooaffentry() parses an affix file entry instead of sscanf(). It 
has a similar algorithm to the parse_affentry() function.


Should I create a new patch to fix this bug (as I did) or this patch 
should go with the patch 
http://www.postgresql.org/message-id/56aa02ee.6090...@postgrespro.ru ?


--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
*** a/src/backend/tsearch/spell.c
--- b/src/backend/tsearch/spell.c
***
*** 458,469  NIAddAffix(IspellDict *Conf, int flag, char flagflags, const char *mask, const c
  }
  
  #define PAE_WAIT_MASK	0
! #define PAE_INMASK	1
  #define PAE_WAIT_FIND	2
! #define PAE_INFIND	3
  #define PAE_WAIT_REPL	4
! #define PAE_INREPL	5
  
  static bool
  parse_affentry(char *str, char *mask, char *find, char *repl)
  {
--- 458,579 
  }
  
  #define PAE_WAIT_MASK	0
! #define PAE_INMASK		1
  #define PAE_WAIT_FIND	2
! #define PAE_INFIND		3
  #define PAE_WAIT_REPL	4
! #define PAE_INREPL		5
! #define PAE_WAIT_TYPE	6
! #define PAE_WAIT_FLAG	7
  
+ /*
+  * Used in parse_ooaffentry() to parse an .affix file entry.
+  */
+ static bool
+ get_nextentry(char **str, char *next)
+ {
+ 	int			state = PAE_WAIT_MASK;
+ 	char	   *pnext = next;
+ 
+ 	*next = '\0';
+ 
+ 	while (**str)
+ 	{
+ 		if (state == PAE_WAIT_MASK)
+ 		{
+ 			if (t_iseq(*str, '#'))
+ return false;
+ 			else if (!t_isspace(*str))
+ 			{
+ COPYCHAR(pnext, *str);
+ pnext += pg_mblen(*str);
+ state = PAE_INMASK;
+ 			}
+ 		}
+ 		else if (state == PAE_INMASK)
+ 		{
+ 			if (t_isspace(*str))
+ 			{
+ *pnext = '\0';
+ return true;
+ 			}
+ 			else
+ 			{
+ COPYCHAR(pnext, *str);
+ pnext += pg_mblen(*str);
+ 			}
+ 		}
+ 		*str += pg_mblen(*str);
+ 	}
+ 
+ 	*pnext ='\0';
+ 
+ 	return *next;
+ }
+ 
+ /*
+  * Parses entry of an .affix file of MySpell or Hunspell format.
+  *
+  * An .affix file entry has the following format:
+  * - header
+  * 
+  * - fields after header:
+  *   
+  */
+ static int
+ parse_ooaffentry(char *str, char *type, char *flag, char *find,
+ char *repl, char *mask)
+ {
+ 	int			state = PAE_WAIT_TYPE,
+ next_state = PAE_WAIT_FLAG;
+ 	int			parse_read = 0;
+ 	bool		valid = true;
+ 
+ 	*type = *flag = *find = *repl = *mask = '\0';
+ 
+ 	while (*str && valid)
+ 	{
+ 		switch (state)
+ 		{
+ 			case PAE_WAIT_TYPE:
+ valid = get_nextentry(, type);
+ break;
+ 			case PAE_WAIT_FLAG:
+ valid = get_nextentry(, flag);
+ next_state = PAE_WAIT_FIND;
+ break;
+ 			case PAE_WAIT_FIND:
+ valid = get_nextentry(, find);
+ next_state = PAE_WAIT_REPL;
+ break;
+ 			case PAE_WAIT_REPL:
+ valid = get_nextentry(, repl);
+ next_state = PAE_WAIT_MASK;
+ break;
+ 			case PAE_WAIT_MASK:
+ get_nextentry(, mask);
+ /* break loop */
+ valid = false;
+ break;
+ 			default:
+ elog(ERROR, "unrecognized state in parse_ooaffentry: %d", state);
+ 		}
+ 		state = next_state;
+ 		if (*str)
+ 			str += pg_mblen(str);
+ 
+ 		parse_read++;
+ 	}
+ 
+ 	return parse_read;
+ }
+ 
+ /*
+  * Parses entry of an .affix file of Ispell format
+  *
+  * An .affix file entry has the following format:
+  *   >  [-,]
+  */
  static bool
  parse_affentry(char *str, char *mask, char *find, char *repl)
  {
***
*** 618,625  NIImportOOAffixes(IspellDict *Conf, const char *filename)
  	int			flag = 0;
  	char		flagflags = 0;
  	tsearch_readline_state trst;
! 	int			scanread = 0;
! 	char		scanbuf[BUFSIZ];
  	char	   *recoded;
  
  	/* read file to find any flag */
--- 728,734 
  	int			flag = 0;
  	char		flagflags = 0;
  	tsearch_readline_state trst;
! 	int			parseread = 0;
  	char	   *recoded;
  
  	/* read file to find any flag */
***
*** 682,689  NIImportOOAffixes(IspellDict *Conf, const char *filename)
  	}
  	tsearch_readline_end();
  
- 	sprintf(scanbuf, "%%6s %%%ds %%%ds %%%ds %%%ds", BUFSIZ / 5, BUFSIZ / 5, BUFSIZ / 5, BUFSIZ / 5);
- 
  	if (!tsearch_readline_begin(, filename))
  		ereport(ERROR,
  (errcode(ERRCODE_CONFIG_FILE_ERROR),
--- 791,796 
***
*** 695,709  NIImportOOAffixes(IspellDict *Conf, const char *filename)
  		if (*recoded == '\0' || t_isspace(recoded) || t_iseq(recoded, '#'))
  			goto nextline;
  
! 		scanread = sscanf(recoded, scanbuf, type, sflag, 

Re: [HACKERS] New committer

[Joshua D. Drake]

On 01/28/2016 06:37 AM, Magnus Hagander wrote:

Hello!

The PostgreSQL core team would like to welcome Dean Rasheed as a new
committer for the PostgreSQL project.

Dean - welcome! Now let's see how quickly you can break the buildfarm!


Congrats!



--
  Magnus Hagander
  Me: http://www.hagander.net/
  Work: http://www.redpill-linpro.com/



--
Command Prompt, Inc.  http://the.postgres.company/
 +1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.


--
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] better systemd integration

[Christoph Berg]

Hi Peter,

thanks for working on this, I'm looking forward to make Debian's
pg_*cluster tools work with that (and hopefully be able to remove tons
of legacy code).

If a cluster is configured for non-hot-standby replication, the
READY=1 seems to never happen. Did you check if that doesn't trigger
any timeouts with would make the unit "fail" or the like?

@@ -2787,6 +2800,10 @@ reaper(SIGNAL_ARGS)
ereport(LOG,
 (errmsg("database system is ready to accept 
connections")));

+#ifdef USE_SYSTEMD
+   sd_notify(0, "READY=1");
+#endif
+
continue;
}

@@ -4930,6 +4947,10 @@ sigusr1_handler(SIGNAL_ARGS)
ereport(LOG,
(errmsg("database system is ready to accept read only 
connections")));

+#ifdef USE_SYSTEMD
+   sd_notify(0, "READY=1");
+#endif
+
pmState = PM_HOT_STANDBY;
/* Some workers may be scheduled to start now */
StartWorkerNeeded = true;


Also, I'm wondering how hard it would be to get socket activation work
with that? (I wouldn't necessarily recommend that for production use,
but on my desktop it would certainly be helpful not to have all those
8.4/9.0/.../9.6 clusters running all the time doing nothing.)

Christoph
-- 
c...@df7cb.de | http://www.df7cb.de/


signature.asc
Description: PGP signature

Re: [HACKERS] New committer

[Robert Haas]

On Thu, Jan 28, 2016 at 9:43 AM, Joshua D. Drake  wrote:
> On 01/28/2016 06:37 AM, Magnus Hagander wrote:
>> The PostgreSQL core team would like to welcome Dean Rasheed as a new
>> committer for the PostgreSQL project.
>>
>> Dean - welcome! Now let's see how quickly you can break the buildfarm!
>
> Congrats!

Woohoo.  Great to have you on board, Dean.

-- 
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] Set search_path + server-prepared statements = cached plan must not change result type

[Vladimir Sitnikov]

Robert>Hmm, so in your example, you actually want replanning to be able to
Robert>change the cached plan's result type?

I want backend to cache _several_ plans behind a single "statement name".
I want to treat "prepare...exec...deallocate" dance as an optimization
step for a simple "exec...exec...exec" sequence.
I do not want to care if "previously prepared query is still valid or
not". For instance, I do not want to check if search_path is still the
same.

Current backend implementation does not report changes to
"search_path", thus clients have no solid way to detect "search_path
changes".

David>Maybe call the new command "PARSE name AS query".

>From JDBC perspective, there is no need in "prepare vs parse" distinction:
1) Explicit "prepare...execute" are not used in typical application code
2) That means, in 99.9% cases, "prepare" would be used by the jdbc driver itself
3) Thus just a single "protocol command" is sufficient.

What I am saying is there are lots of consumers that want to avoid
parsing overhead: plpgsql, pgjdbc, pgjdbc-ng, postgresql-async,
8kdata/phoebe, etc, etc.

All of them will have to deal with search_path vs prepare issue.
If you suggest to deprecate "prepare" in favor of "parse", then all of
the above clients would have to switch to that "parse".
It does not look like a good solution, since lots of existing clients
assume "prepare just works".

If "prepare" command gets deprecated, why "parse" would be better?
What would be the use of "prepare" if all the clients would have to
use "parse" in order to be search_path-compatible?

Vladimir


-- 
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] Template for commit messages

[Alvaro Herrera]

Robert Haas wrote:
> On Thu, Jan 28, 2016 at 6:16 AM, Tomas Vondra
>  wrote:
> >> How about
> >> User-Interface-Bikeshedded-By:
> >> ?
> >
> > +1
> 
> That's sort of implicitly pejorative.   Maybe we could find another
> way to phrase that, like "Design-suggestions-by" or maybe a more
> general "Suggestions-by".

Apologies, I was kidding actually -- I don't think we should list people
on patches just because they provided some input in the thread, but
people that actually reviewed the patch.  Otherwise we risk diluting the
list in the release notes (or wherever it is that we end up crediting
reviewers/contributors), which is also undesirable.

-- 
Á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] New committer

[Oleg Bartunov]

On Thu, Jan 28, 2016 at 5:37 PM, Magnus Hagander 
wrote:

> Hello!
>
> The PostgreSQL core team would like to welcome Dean Rasheed as a new
> committer for the PostgreSQL project.
>
> Dean - welcome! Now let's see how quickly you can break the buildfarm!
>

Congratulations, of course !

I'd like to see next time a short review of a committer, so other
developers could see what they are missing :)


>
> --
>  Magnus Hagander
>  Me: http://www.hagander.net/
>  Work: http://www.redpill-linpro.com/
>

Re: [HACKERS] Template for commit messages

[Tom Lane]

Robert Haas  writes:
> On Thu, Jan 28, 2016 at 8:04 AM, Tomas Vondra
>  wrote:
>> Why can't we do both? That is, have a free-form text with the nuances, and
>> then Reviewed-By listing the main reviewers? The first one is for humans,
>> the other one for automated tools.

> I'm not objecting to or endorsing any specific proposal, just asking
> what we want to do about this.  I think the trick if we do it that way
> will be to avoid having it seem like too much duplication, but there
> may be a way to manage that.

FWIW, I'm a bit suspicious of the idea that we need to make the commit
messages automated-tool-friendly.  What tools are there that would need
to extract this info, and would we trust them if they didn't understand
"nuances"?

I'm on board with Bruce's template as being a checklist of points to be
sure to cover when composing a commit message.  I'm not sure we need
fixed-format rules.

regards, tom lane


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

Re: [HACKERS] [PROPOSAL] VACUUM Progress Checker.

[Amit Langote]

Hi,

On Thu, Jan 28, 2016 at 7:38 PM, Rahila Syed  wrote:
>>+if(!scan_all)
>>+scanned_heap_pages = scanned_heap_pages +
>>next_not_all_visible_block;
>
>>I don't want to be too much of a stickler for details here, but it
>>seems to me that this is an outright lie.
>
> Initially the scanned_heap_pages were meant to report just the scanned pages
> and skipped pages were not added to the count.  Instead the skipped pages
> were deduced from number of total heap pages to be scanned to make the
> number of scanned pages eventually add up to total heap pages.   As per
> comments received later total heap pages were kept constant and skipped
> pages count was added to scanned pages to make the count add up to total
> heap pages at the end of scan. That said, as suggested, scanned_heap_pages
> should be renamed to current_heap_page to report current blkno in
> lazy_scan_heap loop which will add up to total heap pages(nblocks) at the
> end of scan. And scanned_heap_pages can be reported as a separate number
> which wont contain skipped pages.

Or keep scanned_heap_pages as is and add a skipped_pages (or
skipped_heap_pages). I guess the latter would be updated not only for
all visible skipped pages but also pin skipped pages. That is,
updating its counter right after vacrelstats->pinskipped_pages++ which
there are a couple of instances of. Likewise a good (and only?) time
to update the former's counter would be right after
vacrelstats->scanned_pages++. Although, I see at least one place where
both are incremented so maybe I'm not entirely correct about the last
two sentences.

Thanks,
Amit


-- 
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] Several problems in tab-completions for SET/RESET

[Masahiko Sawada]

On Thu, Jan 28, 2016 at 10:15 PM, Michael Paquier
 wrote:
> On Thu, Jan 28, 2016 at 9:32 PM, Fujii Masao  wrote:
>> I found that the following tab-completions for SET/RESET which
>> worked properly before doesn't work properly now in the master.
>>
>> 1. ALTER SYSTEM SET|RESET  lists nothing.
>> 2. ALTER DATABASE xxx SET  lists nothing.
>> 3. ALTER DATABASE xxx SET yyy  lists nothing.
>> 4. ALTER DATABASE xxx SET datestyle TO  lists nothing.
>>
>> Attached patch fixes those problems.
>
> -   else if (Matches4("ALTER", "SYSTEM", "SET|RESET", MatchAny))
> +   else if (Matches3("ALTER", "SYSTEM", "SET|RESET"))
> Good catch.
>
> -   else if (Matches2("SET", MatchAny))
> +   else if (TailMatches2("SET", MatchAny) &&
> +!TailMatches4("UPDATE|DOMAIN", MatchAny,
> MatchAny, MatchAny) &&
> +!TailMatches1("TABLESPACE|SCHEMA") &&
> +!ends_with(prev_wd, ')') &&
> +!ends_with(prev_wd, '='))
> COMPLETE_WITH_CONST("TO");

This change breaks tab completion for ALTER TABLE ... SET
[WITH/LOGGED/UNLOGGED].

It think it should be
> +   else if (Matches2("SET", MatchAny) &&

Related to it, I found tab completion for ALTER TABLE .. SET WITH,
which doesn't working well.
Patch is attached.

Regards,

--
Masahiko Sawada
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 008f3cb..033df74 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1758,6 +1758,8 @@ psql_completion(const char *text, int start, int end)
 	else if (Matches5("ALTER", "TABLE", MatchAny, "SET", "WITHOUT"))
 		COMPLETE_WITH_LIST2("CLUSTER", "OIDS");
 	/* ALTER TABLE  RESET */
+	else if (Matches5("ALTER", "TABLE", MatchAny, "SET", "WITH"))
+		COMPLETE_WITH_CONST("OIDS");
 	else if (Matches4("ALTER", "TABLE", MatchAny, "RESET"))
 		COMPLETE_WITH_CONST("(");
 	/* ALTER TABLE  SET|RESET ( */

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