Re: [HACKERS] Parser extensions (maybe for 10?)

[Tom Lane]

Craig Ringer  writes:
> The other area where there's room for extension without throwing out the
> whole thing and rebuilding is handling of new top-level statements. We can
> probably dispatch the statement text to a sub-parser provided by an
> extension that registers interest in that statement name when we attempt to
> parse it and fail. Even then I'm pretty sure it won't be possible to do so
> while still allowing multi-statements. I wish we didn't support
> multi-statements, but we're fairly stuck with them.

Well, as I said, I've been there and done that.  Things get sticky
when you notice that those "new top-level statements" would like to
contain sub-clauses (e.g. arithmetic expressions) that should be defined
by the core grammar.  And maybe the extension would also like to
define additions to the expression grammar, requiring a recursive
callback into the extension.  It gets very messy very fast.

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] Parser extensions (maybe for 10?)

[Craig Ringer]

On 12 April 2016 at 13:28, David G. Johnston 
wrote:


> ​As recently discovered there is more than one reason why an intelligent
> driver, like the JDBC standard at least requires in a few instances,
> requires knowledge of at least some basic structure​
>
> ​of the statements it sees before sending them off to the server.
>

Indeed.

For one thing, PgJDBC needs to be able to parse the passed SQL text to
extract individual statements and split up multi-statements server-side so
it can bind/parse/execute them separately.

It also has to be able to find placeholders in the query and not be
confused by what might be placeholders if not contained within "identifier
quoting", 'literal quoting' or $q$dollar quoting$q$.

I see almost zero utility in teaching the server about client-side
abstractions like {? = call } . Half the *point* of those is that the
*driver* is supposed to understand them and turn them into *DBMS-specific*
syntax. They're escapes.

Furthermore, and particularly in the JDBC example you provide, my first
> reaction is that it would be a massive encapsulation violation to try and
> get PostgreSQL to understand "{? = call funcname(args)}" and similar higher
> level API specifications.
>

+10

Even if it were easy, it'd be an awful idea. It'd also introduce huge
ambiguities as the mess of umpteen different client parameter-specifier
formats, procedure-call escape formats, etc all clashed in a hideous and
confused mess.

This is the client's job. If the client wants to use %(whatever)s, ?, $1,
:paramname, or Σparam☑ as parameter placeholders we shouldn't have to care.
Same with call-escapes etc. So long as we provide a sensible, well defined
way to do what the client driver needs to do to implement what its clients
expect.

Now, I do think we should one day have a proper CALL statement, but that's
for top-level true stored procedures and unrelated to how the client talks
to the client driver.

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

Re: [HACKERS] Parser extensions (maybe for 10?)

[David G. Johnston]

On Mon, Apr 11, 2016 at 9:58 PM, Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:

> *From:* pgsql-hackers-ow...@postgresql.org [mailto:
> pgsql-hackers-ow...@postgresql.org] * On Behalf Of *Arcadiy Ivanov
>
> Currently the parser and lexer are fully fixed at compile-time and not
> amenable to the extensions - extensions are only capable of introducing
> functions etc.
>
> There is, however, an advantage to being able if not add or alter complete
> statements (which would be nice), but to at least augment portions of
> syntax for existing ones in some places.
>
>
>
> I saw the following discussion in the past, but I haven’t read it:
>
>
>
> Pluggable Parser
>
>
> http://www.postgresql.org/message-id/bf2827dcce55594c8d7a8f7ffd3ab77159878...@szxeml521-mbs.china.huawei.com
>
>
>
> I’m interested in the pluggable, extensible parser for two purposes.  One
> is to add compatibility for other databases.
>
>
>
> The other is for the ODBC (and possibly JDBC) driver.
>
> The ODBC/JDBC specs require some unique syntax constructs, e.g. {? = call
> funcname(arguments)} to call stored procs/functions.  Currently, the
> ODBC/JDBC drivers are forced to parse and convert SQL statements.  It is
> ideal for PostgreSQL itself to understand the ODBC/JDBC syntax, and
> eliminate the burdon of parsing statements from the JDBC/ODBC drivers.
>

​As recently discovered there is more than one reason why an intelligent
driver, like the JDBC standard at least requires in a few instances,
requires knowledge of at least some basic structure​

​of the statements it sees before sending them off to the server.
Furthermore, and particularly in the JDBC example you provide, my first
reaction is that it would be a massive encapsulation violation to try and
get PostgreSQL to understand "{? = call funcname(args)}" and similar higher
level API specifications.

I think PostgreSQL can do quite well by saying, hey this is what we are and
this is what we do.  Compatibility has merit but I'm sure at least some of
those items can make it into the bison files - regardless of whether those
changes end up being accepted into core.  Small-scale forking like this
seems like it would be easier to accomplish if not preferable to making the
entire thing modular.  We have that option to offer others since we are an
open source project.

David J.

Re: [HACKERS] Parser extensions (maybe for 10?)

[Craig Ringer]

On 12 April 2016 at 13:10, Tom Lane  wrote:

>
> I'm interested in possible solutions to this problem, but it's far
> harder than it looks.
>
>
Exactly.

Limited extension points where we accept runtime errors and confine the
extension points can be OK; e.g. SOME STATEMENT ... WITH (THINGY,
OTHER_THINGY) where we allow any series of
identifier|keyword|literal|bareword, accumulate it and pass it as a List of
Node to something else to deal with. Bison can cater to that and similar
structures where the boundaries of the generic/extensible region can be
clearly defined and limited.

The other area where there's room for extension without throwing out the
whole thing and rebuilding is handling of new top-level statements. We can
probably dispatch the statement text to a sub-parser provided by an
extension that registers interest in that statement name when we attempt to
parse it and fail. Even then I'm pretty sure it won't be possible to do so
while still allowing multi-statements. I wish we didn't support
multi-statements, but we're fairly stuck with them.

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

Re: [HACKERS] Parser extensions (maybe for 10?)

[Tom Lane]

Arcadiy Ivanov  writes:
> Is there any interest and/or tips to allow a pluggable parser or at 
> least allow some syntactical pluggability by extensions?

There is a fair amount of discussion of this in the archives.  The short
answer is that bison can't do it, and "let's replace bison" is a proposal
with a steep hill in front of it --- the pain-to-gain ratio is just not
very favorable.

Forty years ago, I worked on systems with extensible parsers at HP,
wherein plug-in extensions could add clauses very similarly to what
you suggest.  Those designs worked, more or less, but they had a lot
of deficiencies; the most fundamental problem being that any parsing
inconsistencies would only appear through misbehavior at runtime,
which you would only discover if you happened to test a case that behaved
oddly *and* notice that it was not giving the result you expected.
Bison is far from perfect on this angle, because %prec declarations can
produce results you weren't expecting ... but it's at least one order of
magnitude better than any extensible-parser solution I've ever seen.
Usually bison will give you a shift/reduce error if you write something
that has more than one possible interpretation.

I'm interested in possible solutions to this problem, but it's far
harder than it looks.

regards, tom lane


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

Re: [HACKERS] [COMMITTERS] pgsql: Allow Pin/UnpinBuffer to operate in a lockfree manner.

[Andres Freund]

On 2016-04-12 00:32:13 -0400, Tom Lane wrote:
> I wrote:
> > This commit has broken buildfarm member gaur, and no doubt pademelon
> > will be equally unhappy once it catches up to HEAD.
> 
> Spoke too soon, actually: pademelon did not get as far as initdb.
> 
> cc: "bufmgr.c", line 4032: error 1521: Incorrect initialization.
> cc: "bufmgr.c", line 4058: error 1521: Incorrect initialization.
> 
> Apparently, assigning the result of init_spin_delay() is not as portable
> as you thought.

Hm. I'm not sure why not though? Is it because delayStatus isn't static
or global scope?


> Maybe convert that into init_spin_delay(SpinDelayStatus *target,
> whatever-the-other-arg-is) ?

Too bad, my compiler generates a bit nicer code for the current version.


Greetings,

Andres Freund


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

Re: [HACKERS] Parser extensions (maybe for 10?)

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org 
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Arcadiy Ivanov

Currently the parser and lexer are fully fixed at compile-time and not amenable 
to the extensions - extensions are only capable of introducing functions etc.

There is, however, an advantage to being able if not add or alter complete 
statements (which would be nice), but to at least augment portions of syntax 
for existing ones in some places.


I saw the following discussion in the past, but I haven’t read it:

Pluggable Parser
http://www.postgresql.org/message-id/bf2827dcce55594c8d7a8f7ffd3ab77159878...@szxeml521-mbs.china.huawei.com

I’m interested in the pluggable, extensible parser for two purposes.  One is to 
add compatibility for other databases.

The other is for the ODBC (and possibly JDBC) driver.
The ODBC/JDBC specs require some unique syntax constructs, e.g. {? = call 
funcname(arguments)} to call stored procs/functions.  Currently, the ODBC/JDBC 
drivers are forced to parse and convert SQL statements.  It is ideal for 
PostgreSQL itself to understand the ODBC/JDBC syntax, and eliminate the burdon 
of parsing statements from the JDBC/ODBC drivers.

Regards
Takayuki Tsunakawa

Re: [HACKERS] Parser extensions (maybe for 10?)

[Craig Ringer]

On 12 April 2016 at 12:36, Arcadiy Ivanov  wrote:


>
> Is there any interest and/or tips to allow a pluggable parser or at least
> allow some syntactical pluggability by extensions?
> I think this may allow some projects to move towards becoming an extension
> as opposed to forking the project entirely.
>

How would you go about it?

PostgreSQL uses a parser generator that produces C code as its output.
Extensions can't just patch the grammar and regenerate the parser. So even
if it were desirable, a fully extensible parser would require a total
rewrite of the parser/lexer.

That doesn't mean there can't be extension points for SQL syntax, they just
need to be planned carefully, located where they won't create parsing
ambiguities, and somewhat limited.

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

[HACKERS] Parser extensions (maybe for 10?)

[Arcadiy Ivanov]

I thought I'd float this idea and see if this gets any traction. Please 
forgive my ignorance if this has been discussed already.


Currently the parser and lexer are fully fixed at compile-time and not 
amenable to the extensions - extensions are only capable of introducing 
functions etc.


There is, however, an advantage to being able if not add or alter 
complete statements (which would be nice), but to at least augment 
portions of syntax for existing ones in some places.


For example PGXL adds the following to CREATE TABLE statement:

[
  DISTRIBUTE BY { REPLICATION | ROUNDROBIN | { [HASH | MODULO ] ( 
column_name ) } } |

  DISTRIBUTED { { BY ( column_name ) } | { RANDOMLY } |
  DISTSTYLE { EVEN | KEY | ALL } DISTKEY ( column_name )
]
[ TO { GROUP groupname | NODE ( nodename [, ... ] ) } ]

Compare:

http://www.postgresql.org/docs/9.5/static/sql-createtable.html
http://files.postgres-xl.org/documentation/sql-createtable.html

Is there any interest and/or tips to allow a pluggable parser or at 
least allow some syntactical pluggability by extensions?
I think this may allow some projects to move towards becoming an 
extension as opposed to forking the project entirely.


Cheers,

--
Arcadiy Ivanov
arca...@gmail.com | @arcivanov | https://ivanov.biz
https://github.com/arcivanov

Re: [HACKERS] [COMMITTERS] pgsql: Allow Pin/UnpinBuffer to operate in a lockfree manner.

[Tom Lane]

I wrote:
> This commit has broken buildfarm member gaur, and no doubt pademelon
> will be equally unhappy once it catches up to HEAD.

Spoke too soon, actually: pademelon did not get as far as initdb.

cc: "bufmgr.c", line 4032: error 1521: Incorrect initialization.
cc: "bufmgr.c", line 4058: error 1521: Incorrect initialization.

Apparently, assigning the result of init_spin_delay() is not as portable
as you thought.  Maybe convert that into
init_spin_delay(SpinDelayStatus *target, whatever-the-other-arg-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] Re: [COMMITTERS] pgsql: Allow Pin/UnpinBuffer to operate in a lockfree manner.

[Andres Freund]

On 2016-04-11 23:59:21 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > Will fix (both initialization and use of pg_atomic_fetch_or_u32), and
> > expand the documentation on why only atomic read/write are supposed to
> > be used.
> 
> FWIW, I'd vote against adding a SpinLockInit there.

Well, it'd not be a SpinLockInit, but a pg_atomic_init_u32(), but ...

> What it would mostly
> do is prevent noticing future mistakes of the same ilk.  It would be
> better no doubt if we didn't have to rely on a nearly-dead platform
> to detect this; but having such detection of a performance bug is better
> than having no detection.

Ok, works for me as well. I guess it'd be useful to add a "modern"
animal that disables spinlocks & atomics...

- Andres


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

Re: [HACKERS] Re: [COMMITTERS] pgsql: Allow Pin/UnpinBuffer to operate in a lockfree manner.

[Tom Lane]

Andres Freund  writes:
>>> The issue is likely that either Alexander or I somehow made
>>> MarkLocalBufferDirty() use pg_atomic_fetch_or_u32(), instead of the
>>> proper pg_atomic_read_u32()/pg_atomic_write_u32().

> Ok, so the theory above fits.

Yah, especially in view of localbuf.c:297 ;-)

> Will fix (both initialization and use of pg_atomic_fetch_or_u32), and
> expand the documentation on why only atomic read/write are supposed to
> be used.

FWIW, I'd vote against adding a SpinLockInit there.  What it would mostly
do is prevent noticing future mistakes of the same ilk.  It would be
better no doubt if we didn't have to rely on a nearly-dead platform
to detect this; but having such detection of a performance bug is better
than having no detection.

regards, tom lane


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

Re: [HACKERS] Re: [COMMITTERS] pgsql: Allow Pin/UnpinBuffer to operate in a lockfree manner.

[Andres Freund]

On 2016-04-11 23:41:10 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > The issue is likely that either Alexander or I somehow made
> > MarkLocalBufferDirty() use pg_atomic_fetch_or_u32(), instead of the
> > proper pg_atomic_read_u32()/pg_atomic_write_u32().
>
> The stack trace I'm seeing is
>
> #5  0x7279cc in elog_finish (elevel=6,
> fmt=0x40057cf8 '\177' ...) at elog.c:1378
> #6  0x5cecd8 in s_lock_stuck (p=0x402995b8, file=0x21bae0 "s_lock.c", line=92)
> at s_lock.c:81
> #7  0x5cedd4 in perform_spin_delay (status=0x7b03b8c8) at s_lock.c:130
> #8  0x5ced40 in s_lock (lock=0x6, file=0x20 ,
> line=6) at s_lock.c:96
> #9  0x53a4b0 in pg_atomic_compare_exchange_u32_impl (ptr=0x402995b8,
> expected=0x5c, newval=58982400) at atomics.c:122
> #10 0x5a280c in MarkLocalBufferDirty (buffer=6)
> at ../../../../src/include/port/atomics/generic.h:224

Ok, so the theory above fits.

Will fix (both initialization and use of pg_atomic_fetch_or_u32), and
expand the documentation on why only atomic read/write are supposed to
be used.

Thanks,

Andres


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

Re: [HACKERS] Re: [COMMITTERS] pgsql: Allow Pin/UnpinBuffer to operate in a lockfree manner.

[Tom Lane]

Andres Freund  writes:
> The issue is likely that either Alexander or I somehow made
> MarkLocalBufferDirty() use pg_atomic_fetch_or_u32(), instead of the
> proper pg_atomic_read_u32()/pg_atomic_write_u32().

The stack trace I'm seeing is

#5  0x7279cc in elog_finish (elevel=6, 
fmt=0x40057cf8 '\177' ...) at elog.c:1378
#6  0x5cecd8 in s_lock_stuck (p=0x402995b8, file=0x21bae0 "s_lock.c", line=92)
at s_lock.c:81
#7  0x5cedd4 in perform_spin_delay (status=0x7b03b8c8) at s_lock.c:130
#8  0x5ced40 in s_lock (lock=0x6, file=0x20 , 
line=6) at s_lock.c:96
#9  0x53a4b0 in pg_atomic_compare_exchange_u32_impl (ptr=0x402995b8, 
expected=0x5c, newval=58982400) at atomics.c:122
#10 0x5a280c in MarkLocalBufferDirty (buffer=6)
at ../../../../src/include/port/atomics/generic.h:224
#11 0x59bba0 in MarkBufferDirty (buffer=6) at bufmgr.c:1489
#12 0x2c9cd0 in heap_multi_insert (relation=0x401c41d0, tuples=0x40201888, 
ntuples=561, cid=0, options=0, bistate=0x40153128) at heapam.c:2760


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

[HACKERS] Re: [COMMITTERS] pgsql: Allow Pin/UnpinBuffer to operate in a lockfree manner.

[Andres Freund]

On 2016-04-11 23:24:36 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > Allow Pin/UnpinBuffer to operate in a lockfree manner.
> 
> This commit has broken buildfarm member gaur, and no doubt pademelon
> will be equally unhappy once it catches up to HEAD.  The reason is that
> you've caused localbuf.c to perform a whole bunch of atomic operations
> on its buffer headers; and on machines that don't have native atomic
> ops, there's a spinlock underlying those; and you did not bother to
> ensure that appropriate SpinLockInit operations happen for local-buffer
> headers.  (HPPA, possibly alone among supported platforms, does not
> think that SpinLockInit is equivalent to memset-to-zeroes.)

That's obviously borked, and need to be fixed.

> While we could fix this by performing suitable SpinLockInit's on
> local-buffer headers, I have to think that that's fundamentally the
> wrong direction.  The *entire* *point* of having local buffers is
> that they are not subject to concurrency overhead.  So IMO, sticking
> atomic-ops calls into localbuf.c is broken on its face.

Note that localbuf.c tries to be careful to only use
pg_atomic_read_u32/pg_atomic_write_u32 - which don't have a concurrency
overhead as they don't utilize atomic ops.

The issue is likely that either Alexander or I somehow made
MarkLocalBufferDirty() use pg_atomic_fetch_or_u32(), instead of the
proper pg_atomic_read_u32()/pg_atomic_write_u32().

Greetings,

Andres Freund


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

Re: [HACKERS] Some other things about contrib/bloom and generic_xlog.c

[Craig Ringer]

On 12 April 2016 at 10:09, Michael Paquier 
wrote:

> On Tue, Apr 12, 2016 at 10:54 AM, Craig Ringer 
> wrote:
>

> > If you rely on shared_preload_libraries, then "oops, I forgot to put
> > myextension in shared_preload_libraries on the downstream" becomes a Bad
> > Thing. There's no way to go back and redo the work you've passed over.
> You
> > have to recreate the standby. Worse, it's silently wrong. The server has
> no
> > idea it was meant to do anything and no way to tell the user it couldn't
> do
> > what it was meant to do.
>
> Well, one playing with the generic WAL record facility is
>

Yeah... that's what people say about a lot of things.

If it's there, someone will shoot their foot off with it and blame us.

There's a point where you have to say "well, that was dumb, you had to take
the safety off, remove the barrel plug attached to the giant sign saying
'do not remove', find and insert the bolt, and load the ammunition yourself
first, maybe you shouldn't have done that?"

However, that doesn't mean we should make it easy for a simple oversight to
have serious data correctness implications. I'm on the fence about whether
this counts; enough so that I wouldn't fight hard to get it in even if a
patch existed, which it doesn't, and we weren't past feature freeze, which
we are.



> Any other
> infrastructure is going to be one brick shy of a load


I disagree. It's very useful, just not for what you (and I) want to use it
for. It's still quite reasonable for custom index representations, and it
can be extended later.


> We could for example force the
> hook to set some validation flags that are then checked in the generic
> redo routine, and mark in the WAL record itself that this record
> should have used the hook. If the record is replayed and this hook is
> missing, then do a FATAL and prevent recovery to move on.
>
>
Right, but _which_ routine on the standby? How does the standby know which
hook must be called? You can't just say "any hook"; there might be multiple
ones interested in different generic WAL messages. You need a registration
system of some sort and a way for the standby and master to agree on how to
identify extensions that have redo hooks for generic WAL. Then you need to
be able to look up that registration in some manner during redo.

The simplest registration system would be something like a function called
at _PG_init time that passes a globally-unique integer identifier for the
extension that maps to some kind of central web-based registry where we
hand out IDs. Forever. Then the standby says "this xlog needs extension
with generic xlog id 42 but it's missing". But we all know how well those
generic global identifier systems work when people are testing and
prototyping stuff, want to change versions incompatibly, etc

So I disagree it's as simple as a validation flag.

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

Re: [HACKERS] [COMMITTERS] pgsql: Allow Pin/UnpinBuffer to operate in a lockfree manner.

[Tom Lane]

Andres Freund  writes:
> Allow Pin/UnpinBuffer to operate in a lockfree manner.

This commit has broken buildfarm member gaur, and no doubt pademelon
will be equally unhappy once it catches up to HEAD.  The reason is that
you've caused localbuf.c to perform a whole bunch of atomic operations
on its buffer headers; and on machines that don't have native atomic
ops, there's a spinlock underlying those; and you did not bother to
ensure that appropriate SpinLockInit operations happen for local-buffer
headers.  (HPPA, possibly alone among supported platforms, does not
think that SpinLockInit is equivalent to memset-to-zeroes.)

While we could fix this by performing suitable SpinLockInit's on
local-buffer headers, I have to think that that's fundamentally the
wrong direction.  The *entire* *point* of having local buffers is
that they are not subject to concurrency overhead.  So IMO, sticking
atomic-ops calls into localbuf.c is broken on its face.

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] Move PinBuffer and UnpinBuffer to atomics

[Andres Freund]

On 2016-04-11 14:40:29 -0700, Andres Freund wrote:
> On 2016-04-11 12:17:20 -0700, Andres Freund wrote:
> I did get access to the machine (thanks!). My testing shows that
> performance is sensitive to various parameters influencing memory
> allocation. E.g. twiddling with max_connections changes
> performance. With max_connections=400 and the previous patches applied I
> get ~122 tps, with 402 ~162 tps.  This sorta confirms that we're
> dealing with an alignment/sharing related issue.
> 
> Padding PGXACT to a full cache-line seems to take care of the largest
> part of the performance irregularity. I looked at perf profiles and saw
> that most cache misses stem from there, and that the percentage (not
> absolute amount!) changes between fast/slow settings.
> 
> To me it makes intuitive sense why you'd want PGXACTs to be on separate
> cachelines - they're constantly dirtied via SnapshotResetXmin(). Indeed
> making it immediately return propels performance up to 172, without
> other changes. Additionally cacheline-padding PGXACT speeds things up to
> 175 tps.
> 
> But I'm unclear why the magnitude of the effect depends on other
> allocations. With the previously posted patches allPgXact is always
> cacheline-aligned.

I've spent considerable amount experimenting around this. The alignment
of allPgXact does *not* apear to play a significant role; rather it
apears to be the the "distance" between the allPgXact and pgprocno
arrays.

Alexander, could you post dmidecode output, and install numactl &
numastat on the machine? I wonder if the box has cluster-on-die
activated or not.  Do I see correctly that this is a system that could
potentially have 8 sockets, but actually has only four? Because I see
physical id : 3 in /proc/cpuinfo only going up to three (from zero),
not 7? And there's only 144 processorcs, while each E7-8890 v3 should
have 36 threads.

Greetings,

Andres Freund


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

Re: [HACKERS] Some other things about contrib/bloom and generic_xlog.c

[Michael Paquier]

On Tue, Apr 12, 2016 at 10:54 AM, Craig Ringer  wrote:
> On 12 April 2016 at 08:36, Michael Paquier wrote:
> The only way we really have at the moment is shared_preload_libraries.

That's exactly my point.

> If you rely on shared_preload_libraries, then "oops, I forgot to put
> myextension in shared_preload_libraries on the downstream" becomes a Bad
> Thing. There's no way to go back and redo the work you've passed over. You
> have to recreate the standby. Worse, it's silently wrong. The server has no
> idea it was meant to do anything and no way to tell the user it couldn't do
> what it was meant to do.

Well, one playing with the generic WAL record facility is

> We can't register redo routines in the catalogs because we don't have access
> to the syscaches etc during redo (right?).

Yes, the startup process does not have that knowledge.

> So how do we look at a generic log record, say "ok, the upstream that wrote
> this says it needs to invoke registered generic xlog hook 42, which is
>  from  at " ?
>
> Personally I'd be willing to accept the limitations of the s_p_l and hook
> approach to the point where I think we should add the hook and accept the
> caveats of its use ... but I understand the problems with it.

Honestly, so do I, and I could accept the use of a hook in
generic_redo in this code path which is really... Generic. Any other
infrastructure is going to be one brick shy of a load, and we are
actually sure that those routines are getting loaded once we reach the
first redo routine as far as I know. We could for example force the
hook to set some validation flags that are then checked in the generic
redo routine, and mark in the WAL record itself that this record
should have used the hook. If the record is replayed and this hook is
missing, then do a FATAL and prevent recovery to move on.
-- 
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] Some other things about contrib/bloom and generic_xlog.c

[Craig Ringer]

On 12 April 2016 at 08:36, Michael Paquier 
wrote:


>
> > Also, the entire point here is that extensions DON'T
> > get to have custom apply routines, because we have no way for
> > replay to know about such apply routines.  (It surely cannot look
> > in the catalogs for them, but there's no other suitable infrastructure
> > either.)  In turn, that means there's little value in having any custom
> > data associated with the WAL records.
>
> Well, yes, the startup process has no knowledge of that. That's why it
> would need to go through a hook, but the startup process has no
> knowledge of routines loaded via _PG_init perhaps?


The only way we really have at the moment is shared_preload_libraries.

This got discussed much earlier, possibly on a previous iteration of the
generic xlog discussions rather than this specific thread. IIRC the gist
was that we need a way to:

- Flag the need for a redo routine so that replay stops if that redo
routine isn't available
- Find out *which* redo routine the generic log message needs during redo
- Get a pointer to that routine to actually call it

... and also possibly allow the admin to force skip of redo calls for a
given extension (but not others) in case of a major problem.

If you rely on shared_preload_libraries, then "oops, I forgot to put
myextension in shared_preload_libraries on the downstream" becomes a Bad
Thing. There's no way to go back and redo the work you've passed over. You
have to recreate the standby. Worse, it's silently wrong. The server has no
idea it was meant to do anything and no way to tell the user it couldn't do
what it was meant to do.

We can't register redo routines in the catalogs because we don't have
access to the syscaches etc during redo (right?).

So how do we look at a generic log record, say "ok, the upstream that wrote
this says it needs to invoke registered generic xlog hook 42, which is
 from  at " ?

Personally I'd be willing to accept the limitations of the s_p_l and hook
approach to the point where I think we should add the hook and accept the
caveats of its use ... but I understand the problems with it. I understand
not having custom redo routine support in this iteration of the patch. It's
somewhat orthogonal job to this patch anyway - while handy for specific
relation generic xlog, custom redo routines make most sense when combined
with generic xlog that isn't necessarily associated with a specific
relation. This patch doesn't support that.

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

Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW

[Michael Paquier]

On Mon, Apr 11, 2016 at 5:16 PM, Etsuro Fujita
 wrote:
> On 2016/04/11 12:30, Michael Paquier wrote:
>>
>> +   if ((cancel = PQgetCancel(entry->conn)))
>> +   {
>> +   PQcancel(cancel, errbuf, sizeof(errbuf));
>> +   PQfreeCancel(cancel);
>> +   }
>> Wouldn't it be better to issue a WARNING here if PQcancel does not
>> succeed?
>
> Seems like a good idea.  Attached is an updated version of the patch.

Thanks for the new version. The patch looks good to me.
-- 
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] Lets (not) break all the things. Was: [pgsql-advocacy] 9.6 -> 10.0

[Craig Ringer]

On 12 April 2016 at 00:39, Justin Clift  wrote:

> Moving over a conversation from the pgsql-advocacy mailing list.  In it
> Simon (CC'd) raised the issue of potentially creating a
> backwards-compatibility
> breaking release at some point in the future, to deal with things that
> might have no other solution (my wording).
>
> Relevant part of that thread there for reference:
>
>
> http://www.postgresql.org/message-id/CANP8+jLtk1NtaJyXc=hAqX=0k+ku4zfavgvbkfs+_sor9he...@mail.gmail.com
>
> Simon included a short starter list of potentials which might be in
> that category:
>
>   * SQL compliant identifiers
>   * Remove RULEs
>   * Change recovery.conf
>   * Change block headers
>   * Retire template0, template1
>   * Optimise FSM
>   * Add heap metapage
>   * Alter tuple headers
>   et al
>

+

* v4 protocol (feature negotiation, lazy blob fetching, etc)
* retire pg_hba.conf and use SQL access management

?

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

Re: [HACKERS] Some other things about contrib/bloom and generic_xlog.c

[Michael Paquier]

On Tue, Apr 12, 2016 at 9:33 AM, Tom Lane  wrote:
> ... BTW, with respect to the documentation angle, it seems to me
> that it'd be better if GenericXLogRegister were renamed to
> GenericXLogRegisterBuffer, or perhaps GenericXLogRegisterPage.
> I think this would make the documentation clearer, and it would
> also make it easier to add other sorts of Register actions later,
> if we ever think of some (which seems not unlikely, really).

Funny thing. I just suggested the same just above :) With a second
routine to generate a delta difference from a page to keep the
knowledge of this delta in its own code path.

> Another thing to think about is whether we're going to regret
> hard-wiring the third argument as a boolean.  Should we consider
> making it a bitmask of flags, instead?  It's not terribly hard
> to think of other flags we might want there in future; for example
> maybe something to tell GenericXLogFinish whether it's worth trying
> to identify data movement on the page rather than just doing the
> byte-by-byte delta calculation.

Yes. Definitely this interface needs more thoughts. I'd think of
GenericXLogFinish as a more generic entry point.
-- 
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] Some other things about contrib/bloom and generic_xlog.c

[Michael Paquier]

On Tue, Apr 12, 2016 at 9:21 AM, Tom Lane  wrote:
> Michael Paquier  writes:
>> What I thought we should be able to do with that should not be only
>> limited to indexes, aka to:
>> - Be able to register and log full pages
>> - Be able to log data associated to a block
>> - Be able to have record data
>> - Use at recovery custom routines to apply those WAL records
>
> I'm not following your distinction between a "page" and a "block",
> perhaps.

s/block/page/. Sorry for the confusion.

> Also, the entire point here is that extensions DON'T
> get to have custom apply routines, because we have no way for
> replay to know about such apply routines.  (It surely cannot look
> in the catalogs for them, but there's no other suitable infrastructure
> either.)  In turn, that means there's little value in having any custom
> data associated with the WAL records.

Well, yes, the startup process has no knowledge of that. That's why it
would need to go through a hook, but the startup process has no
knowledge of routines loaded via _PG_init perhaps? I recall that it
loaded them. The weakness with this interface is that one needs to be
sure that this hook is actually loaded properly.

> If we ever solve the registering-custom-replay-routines conundrum,
> it'll be time to think about what the frontend API for that ought
> to be.  But this patch is not pretending to solve that, and indeed is
> mainly showing that it's possible to have a useful WAL extension API
> that doesn't require solving it.

Yep. I am not saying the contrary. This patch solves its own category
of things with its strict page-level control.

> In any case, the existence of this API doesn't foreclose adding
> other APIs (perhaps backed by other RM_GENERIC_ID WAL record types)
> later on.  So I don't think we need to insist that it do everything
> anyone will ever want.

Perhaps I am just confused by the interface. Linking the idea of a
page delta with GenericXLogRegister is not that intuitive, knowing
that what it should actually be GenericXLogRegisterBuffer and we
should have GenericXLogAddDelta, that applies a page difference on top
of an existing one.
-- 
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] Some other things about contrib/bloom and generic_xlog.c

[Tom Lane]

... BTW, with respect to the documentation angle, it seems to me
that it'd be better if GenericXLogRegister were renamed to
GenericXLogRegisterBuffer, or perhaps GenericXLogRegisterPage.
I think this would make the documentation clearer, and it would
also make it easier to add other sorts of Register actions later,
if we ever think of some (which seems not unlikely, really).

Another thing to think about is whether we're going to regret
hard-wiring the third argument as a boolean.  Should we consider
making it a bitmask of flags, instead?  It's not terribly hard
to think of other flags we might want there in future; for example
maybe something to tell GenericXLogFinish whether it's worth trying
to identify data movement on the page rather than just doing the
byte-by-byte delta calculation.

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] Some other things about contrib/bloom and generic_xlog.c

[Tom Lane]

Michael Paquier  writes:
> As given out now, we are able to do the following:
> - Log a full page
> - Log a delta of a full page, which is actually data associated to a page.
> - At recovery, replay those full pages with a delta.

Right.

> What I thought we should be able to do with that should not be only
> limited to indexes, aka to:
> - Be able to register and log full pages
> - Be able to log data associated to a block
> - Be able to have record data
> - Use at recovery custom routines to apply those WAL records

I'm not following your distinction between a "page" and a "block",
perhaps.  Also, the entire point here is that extensions DON'T
get to have custom apply routines, because we have no way for
replay to know about such apply routines.  (It surely cannot look
in the catalogs for them, but there's no other suitable infrastructure
either.)  In turn, that means there's little value in having any custom
data associated with the WAL records.

If we ever solve the registering-custom-replay-routines conundrum,
it'll be time to think about what the frontend API for that ought
to be.  But this patch is not pretending to solve that, and indeed is
mainly showing that it's possible to have a useful WAL extension API
that doesn't require solving it.

In any case, the existence of this API doesn't foreclose adding
other APIs (perhaps backed by other RM_GENERIC_ID WAL record types)
later on.  So I don't think we need to insist that it do everything
anyone will ever want.

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] Some other things about contrib/bloom and generic_xlog.c

[Michael Paquier]

On Mon, Apr 11, 2016 at 11:29 PM, Tom Lane  wrote:
> Michael Paquier  writes:
>> Actually, as I look at this code for the first time, I find that
>> GenericXLogFinish() is a very confusing interface. It makes no
>> distinction between a page and the meta data associated to this page,
>> this is controlled by a flag isNew and buffer data is saved as some
>> delta. Actually, fullmage is not exactly true, this may refer to a
>> page that has a hole. It seems to me that we should not have one but
>> two routines: GenericXLogRegisterBuffer and
>> GenericXLogRegisterBufData, similar to what the normal XLOG routines
>> offer.
>
> Hmm.  Maybe the documentation leaves something to be desired, but
> I thought that the interface was reasonable if you grant that the
> goal is to be easy to use rather than to have maximum performance.
> The calling code only has to concern itself with making the actual
> changes to the target pages, not with constructing WAL descriptions
> of those changes.  Also, the fact that the changes don't have to be
> made within a critical section is a bigger help than it might sound.
>
> So I don't really have any problems with the API as such.  I tried
> to improve the docs a day or two ago, but maybe that needs further
> work.

Well, I am not saying that the given interface does nothing, far from
that. Though I think that we could take advantage of the rmgr
RM_GENERIC_ID introduced by this interface to be able to generate
custom WAL records and pass them through a stream.

As given out now, we are able to do the following:
- Log a full page
- Log a delta of a full page, which is actually data associated to a page.
- At recovery, replay those full pages with a delta.

What I thought we should be able to do with that should not be only
limited to indexes, aka to:
- Be able to register and log full pages
- Be able to log data associated to a block
- Be able to have record data
- Use at recovery custom routines to apply those WAL records
The first 3 ones are done via the set of routines generating WAL
records for the rmgr RM_GENERIC_ID. The 4th one needs a hook in
generic_redo. Looking at the thread where this patch has been debated
I am not seeing a discussion regarding 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] 'Create table if not exists as' breaks SPI_execute

[Tom Lane]

Stas Kelvich  writes:
> SPI_execute assumes that CreateTableAsStmt always have completionTag == 
> “completionTag”.
> But it isn’t true in case of ‘IF NOT EXISTS’ present.

Pushed, thanks.

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

[Masahiko Sawada]

On Mon, Apr 11, 2016 at 8:47 PM, Fujii Masao  wrote:
> On Mon, Apr 11, 2016 at 5:52 PM, Masahiko Sawada  
> wrote:
>> On Mon, Apr 11, 2016 at 1:31 PM, Fujii Masao  wrote:
>>> On Mon, Apr 11, 2016 at 10:58 AM, Masahiko Sawada  
>>> wrote:
 On Sat, Apr 9, 2016 at 12:32 PM, Tom Lane  wrote:
> Jeff Janes  writes:
>> When I compile now without cassert, I get the compiler warning:
>
>> syncrep.c: In function 'SyncRepUpdateConfig':
>> syncrep.c:878:6: warning: variable 'parse_rc' set but not used
>> [-Wunused-but-set-variable]
>
> If there's a good reason for that to be an Assert, I don't see it.
> There are no callers of SyncRepUpdateConfig that look like they
> need to, or should expect not to have to, tolerate errors.
> I think the way to fix this is to turn the Assert into a plain
> old test-and-ereport-ERROR.
>

 I've changed the draft patch Amit implemented so that it doesn't parse
 twice(check_hook and assign_hook).
 So assertion that was in assign_hook is no longer necessary.

 Please find attached.
>>>
>>> Thanks for the patch!
>>>
>>> When I emptied s_s_names, reloaded the configration file, set it to 
>>> 'standby1'
>>> and reloaded the configuration file again, the master crashed with
>>> the following error.
>>>
>>> *** glibc detected *** postgres: wal sender process postgres [local]
>>> streaming 0/3015F18: munmap_chunk(): invalid pointer:
>>> 0x024d9a40 ***
>>> === Backtrace: =
>>> *** glibc detected *** postgres: wal sender process postgres [local]
>>> streaming 0/3015F18: munmap_chunk(): invalid pointer:
>>> 0x024d9a40 ***
>>> /lib64/libc.so.6[0x3be8e75f4e]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x97dae2]
>>> === Backtrace: =
>>> /lib64/libc.so.6[0x3be8e75f4e]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(set_config_option+0x12cb)[0x982242]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(SetConfigOption+0x4b)[0x9827ff]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x97dae2]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x988b4e]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(set_config_option+0x12cb)[0x982242]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x98af40]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(SetConfigOption+0x4b)[0x9827ff]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(ProcessConfigFile+0x9f)[0x98a98b]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x988b4e]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x98af40]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7b50fd]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(ProcessConfigFile+0x9f)[0x98a98b]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7b359c]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7b50fd]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(exec_replication_command+0x1a7)[0x7b47b6]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7b359c]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(PostgresMain+0x772)[0x8141b6]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(exec_replication_command+0x1a7)[0x7b47b6]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7896f7]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(PostgresMain+0x772)[0x8141b6]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x788e62]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7896f7]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x785544]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x788e62]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(PostmasterMain+0x1134)[0x784c08]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x785544]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x6ce12e]
>>> /lib64/libc.so.6(__libc_start_main+0xfd)[0x3be8e1ed5d]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(PostmasterMain+0x1134)[0x784c08]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x467e99]
>>>
>>
>> Thank you for reviewing.
>>
>> SyncRepUpdateConfig() seems to be no longer necessary.
>
> Really? I was thinking that something like that function needs to
> be called at the beginning of a backend and walsender in
> EXEC_BACKEND case. No?
>

Hmm, in EXEC_BACKEND case, I guess that 

Re: [HACKERS] Preprocessor condition fix

[Tom Lane]

Christian Ullrich  writes:
> Here is a one-line patch to fix a wrong preprocessor condition in 
> pg_regress, found because the VS 2015 compiler warns on the cast in the 
> 32-bit branch where apparently earlier versions did not.

Pushed, thanks.

> According to git grep, this is the only place where WIN64 is used 
> without the leading underscore.

Hm, my grep found another one ...

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] Removing the TRACE_SORT macro

[Peter Geoghegan]

On Mon, Apr 11, 2016 at 2:54 PM, Robert Haas  wrote:
> I'm not excited about this change.  It doesn't really buy us anything
> that I can see.  For one thing, I think we've arguably got too much
> trace_sort output from sorts now and should look to reduce that
> instead of further normalizing it.  For another thing, the only
> advantage to removing it is that it makes it easier to add even more,
> and we're not going to do that in 9.6.  If we decide to do it in 9.7,
> we can consider the proposal to remove TRACE_SORT calls then.  There
> is just nothing urgent about this.

Of course it isn't urgent. I seem to have significant difficulty
getting proposals accepted that are not urgent, even if they are
important. While this proposal isn't important, it also isn't hard or
in any way risky or time consuming. I just want to get rid of the
TRACE_SORT crud. That's all.

I'll put it on my long-term TODO list.

-- 
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] Move PinBuffer and UnpinBuffer to atomics

[Andres Freund]

On 2016-04-11 14:40:29 -0700, Andres Freund wrote:
> On 2016-04-11 12:17:20 -0700, Andres Freund wrote:
> > On 2016-04-11 22:08:15 +0300, Alexander Korotkov wrote:
> > > On Mon, Apr 11, 2016 at 5:04 PM, Alexander Korotkov <
> > > a.korot...@postgrespro.ru> wrote:
> > > 
> > > > On Mon, Apr 11, 2016 at 8:10 AM, Andres Freund  
> > > > wrote:
> > > >
> > > >> Could you retry after applying the attached series of patches?
> > > >>
> > > >
> > > > Yes, I will try with these patches and snapshot too old reverted.
> > > >
> > > 
> > > I've run the same benchmark with 279d86af and 848ef42b reverted.  I've
> > > tested of all 3 patches from you applied and, for comparison, 3 patches +
> > > clog buffers reverted back to 32.
> > > 
> > > clients patches patches + clog_32
> > > 1 12594   12556
> > > 2 26705   26258
> > > 4 50985   53254
> > > 8103234  104416
> > > 10   135321  130893
> > > 20   268675  267648
> > > 30   370437  409710
> > > 40   486512  482382
> > > 50   539910  525667
> > > 60   616401  672230
> > > 70   667864  660853
> > > 80   924606  737768
> > > 90  1217435  799581
> > > 100 1326054  863066
> > > 110 1446380  980206
> > > 120 1484920 1000963
> > > 130 1512440 1058852
> > > 140 1536181 1088958
> > > 150 1504750 1134354
> > > 160 1461513 1132173
> > > 170 1453943 1158656
> > > 180 1426288 1120511
> 
> > Any chance that I could run some tests on that machine myself? It's very
> > hard to investigate that kind of issue without access; the only thing I
> > otherwise can do is lob patches at you, till we find the relevant
> > memory.
> 
> I did get access to the machine (thanks!). My testing shows that
> performance is sensitive to various parameters influencing memory
> allocation. E.g. twiddling with max_connections changes
> performance. With max_connections=400 and the previous patches applied I
> get ~122 tps, with 402 ~162 tps.  This sorta confirms that we're
> dealing with an alignment/sharing related issue.
> 
> Padding PGXACT to a full cache-line seems to take care of the largest
> part of the performance irregularity. I looked at perf profiles and saw
> that most cache misses stem from there, and that the percentage (not
> absolute amount!) changes between fast/slow settings.
> 
> To me it makes intuitive sense why you'd want PGXACTs to be on separate
> cachelines - they're constantly dirtied via SnapshotResetXmin(). Indeed
> making it immediately return propels performance up to 172, without
> other changes. Additionally cacheline-padding PGXACT speeds things up to
> 175 tps.
> 
> But I'm unclear why the magnitude of the effect depends on other
> allocations. With the previously posted patches allPgXact is always
> cacheline-aligned.

Oh, one more thing: The volatile on PGXACT in GetSnapshotData() costs us
about 100k tps on that machine; without, afaics, any point but force
pgxact->xmin to only be loaded once (which a *((volatile
TransactionId)>xmin) does just as well).

Greetings,

Andres Freund


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

Re: [HACKERS] Removing the TRACE_SORT macro

[Robert Haas]

On Mon, Apr 11, 2016 at 4:43 PM, Peter Geoghegan  wrote:
> Currently, debug instrumentation for sorting is only available if the
> TRACE_SORT macro was defined when PostgreSQL was compiled. It is
> defined by default, and so in practice it's always available; there is
> really no upside to disabling it. "18.17. Developer Options" notes
> this restriction for trace_sort, which is the only such restriction.
>
> The number of TRACE_SORT elog() logging callers has grown
> significantly in the past couple of releases. The associated "#ifdef
> TRACE_SORT" crud has also grown.
>
> I propose that we completely remove the TRACE_SORT macro, and all the
> associated crud. Just having that as an option suggests that there is
> some possible upside to disabling trace_sort, which is clearly not
> true. I will write a patch doing this if there are no objections. I
> think this is justifiable as clean-up for 9.6.

I'm not excited about this change.  It doesn't really buy us anything
that I can see.  For one thing, I think we've arguably got too much
trace_sort output from sorts now and should look to reduce that
instead of further normalizing it.  For another thing, the only
advantage to removing it is that it makes it easier to add even more,
and we're not going to do that in 9.6.  If we decide to do it in 9.7,
we can consider the proposal to remove TRACE_SORT calls then.  There
is just nothing urgent about this.

-- 
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] Removing the TRACE_SORT macro

[Peter Geoghegan]

On Mon, Apr 11, 2016 at 2:35 PM, Simon Riggs  wrote:
> Yeh, sort has changed enough now that fixes weren't going to backpatch
> cleanly, so its a good time to do cleanup.

I wonder if the category of "Developer Options" is appropriate for
trace_sort. trace_sort is closer to log_executor_stats, which is
categorized as "Statistics / Monitoring". I guess it isn't worth
changing now, though.

-- 
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] Choosing parallel_degree

[Simon Riggs]

On 8 April 2016 at 17:49, Robert Haas  wrote:


> With the patch, you can - if you wish - substitute
> some other number for the one the planner comes up with.


I saw you're using AccessExclusiveLock, the reason being it affects SELECTs.

That is supposed to apply when things might change the answer from a
SELECT, whereas this affects only the default for a plan.

Can I change this to a lower setting? I would have done this before
applying the patch, but you beat me to it.

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

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

Re: [HACKERS] Choosing parallel_degree

[Simon Riggs]

On 8 April 2016 at 17:27, Paul Ramsey  wrote:


> PostGIS is just one voice...
>

We're listening.


> >> Functions have very unequal CPU costs, and we're talking here about
> >> using CPUs more effectively, why are costs being given the see-no-evil
> >> treatment? This is as true in core as it is in PostGIS, even if our
> >> case is a couple orders of magnitude more extreme: a filter based on a
> >> complex combination of regex queries will use an order of magnitude
> >> more CPU than one that does a little math, why plan and execute them
> >> like they are the same?
> >
> > Functions have user assignable costs.
>
> We have done a relatively bad job of globally costing our functions
> thus far, because it mostly didn't make any difference. In my testing
> [1], I found that costing could push better plans for parallel
> sequence scans and parallel aggregates, though at very extreme cost
> values (1000 for sequence scans and 1 for aggregates)
>
> Obviously, if costs can make a difference for 9.6 and parallelism
> we'll rigorously ensure we have good, useful costs. I've already
> costed many functions in my parallel postgis test branch [2]. Perhaps
> the avoidance of cost so far is based on the relatively nebulous
> definition it has: about the only thing in the docs is "If the cost is
> not specified, 1 unit is assumed for C-language and internal
> functions, and 100 units for functions in all other languages. Larger
> values cause the planner to try to avoid evaluating the function more
> often than necessary."
>
> So what about C functions then? Should a string comparison be 5 and a
> multiplication 1? An image histogram 1000?
>

We don't have a clear methodology for how to do this.

It's a single parameter to allow you to achieve the plans that work
optimally. Hopefully that is simple enough for everyone to use and yet
flexible enough to make a difference.

If its not what you need, show us and it may make the case for change.

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

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

Re: [HACKERS] Move PinBuffer and UnpinBuffer to atomics

[Andres Freund]

On 2016-04-11 12:17:20 -0700, Andres Freund wrote:
> On 2016-04-11 22:08:15 +0300, Alexander Korotkov wrote:
> > On Mon, Apr 11, 2016 at 5:04 PM, Alexander Korotkov <
> > a.korot...@postgrespro.ru> wrote:
> > 
> > > On Mon, Apr 11, 2016 at 8:10 AM, Andres Freund  wrote:
> > >
> > >> Could you retry after applying the attached series of patches?
> > >>
> > >
> > > Yes, I will try with these patches and snapshot too old reverted.
> > >
> > 
> > I've run the same benchmark with 279d86af and 848ef42b reverted.  I've
> > tested of all 3 patches from you applied and, for comparison, 3 patches +
> > clog buffers reverted back to 32.
> > 
> > clients patches patches + clog_32
> > 1 12594   12556
> > 2 26705   26258
> > 4 50985   53254
> > 8103234  104416
> > 10   135321  130893
> > 20   268675  267648
> > 30   370437  409710
> > 40   486512  482382
> > 50   539910  525667
> > 60   616401  672230
> > 70   667864  660853
> > 80   924606  737768
> > 90  1217435  799581
> > 100 1326054  863066
> > 110 1446380  980206
> > 120 1484920 1000963
> > 130 1512440 1058852
> > 140 1536181 1088958
> > 150 1504750 1134354
> > 160 1461513 1132173
> > 170 1453943 1158656
> > 180 1426288 1120511

> Any chance that I could run some tests on that machine myself? It's very
> hard to investigate that kind of issue without access; the only thing I
> otherwise can do is lob patches at you, till we find the relevant
> memory.

I did get access to the machine (thanks!). My testing shows that
performance is sensitive to various parameters influencing memory
allocation. E.g. twiddling with max_connections changes
performance. With max_connections=400 and the previous patches applied I
get ~122 tps, with 402 ~162 tps.  This sorta confirms that we're
dealing with an alignment/sharing related issue.

Padding PGXACT to a full cache-line seems to take care of the largest
part of the performance irregularity. I looked at perf profiles and saw
that most cache misses stem from there, and that the percentage (not
absolute amount!) changes between fast/slow settings.

To me it makes intuitive sense why you'd want PGXACTs to be on separate
cachelines - they're constantly dirtied via SnapshotResetXmin(). Indeed
making it immediately return propels performance up to 172, without
other changes. Additionally cacheline-padding PGXACT speeds things up to
175 tps.

But I'm unclear why the magnitude of the effect depends on other
allocations. With the previously posted patches allPgXact is always
cacheline-aligned.

Greetings,

Andres Freund


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

Re: [HACKERS] Remove unused function from walsender.c

[Simon Riggs]

On 11 April 2016 at 08:05, Fujii Masao  wrote:


> There is an unused function GetOldestWALSendPointer() in walsender.c.
> Per comment, it was introduced because we may need it in the future for
> synchronous replication.
>
> Now we have very similar function SyncRepGetOldestSyncRecPtr() in
> syncrep.c. Which makes me think that GetOldestWALSendPointer()
> no longer needs to be maintained. So, is it time to remove that unused
> function?
>

Seems sensible cleanup to me.

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

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

Re: [HACKERS] Removing the TRACE_SORT macro

[Simon Riggs]

On 11 April 2016 at 21:43, Peter Geoghegan  wrote:

> Currently, debug instrumentation for sorting is only available if the
> TRACE_SORT macro was defined when PostgreSQL was compiled. It is
> defined by default, and so in practice it's always available; there is
> really no upside to disabling it. "18.17. Developer Options" notes
> this restriction for trace_sort, which is the only such restriction.
>
> The number of TRACE_SORT elog() logging callers has grown
> significantly in the past couple of releases. The associated "#ifdef
> TRACE_SORT" crud has also grown.
>
> I propose that we completely remove the TRACE_SORT macro, and all the
> associated crud. Just having that as an option suggests that there is
> some possible upside to disabling trace_sort, which is clearly not
> true. I will write a patch doing this if there are no objections. I
> think this is justifiable as clean-up for 9.6.
>

Yeh, sort has changed enough now that fixes weren't going to backpatch
cleanly, so its a good time to do cleanup.

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

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

Re: [HACKERS] Choosing parallel_degree

[Julien Rouhaud]

On 11/04/2016 17:44, Robert Haas wrote:
> On Mon, Apr 11, 2016 at 11:27 AM, Julien Rouhaud
>  wrote:
>> On 11/04/2016 15:56, tushar wrote:
>>>
>>> I am assuming parallel_degree=0 is as same as not using it  , i.e
>>> create table fok2(n int) with (parallel_degree=0);  = create table
>>> fok2(n int);
>>>
>>> so in this case it should have accepted the parallel seq .scan.
>>>
>>
>> No, setting it to 0 means to force not using parallel workers (but
>> considering the parallel path IIRC).
> 
> I'm not sure what the parenthesized bit means, because you can't use
> parallelism without workers.

Obvious mistake, sorry.

>  But I think I should have made the docs
> more clear that 0 = don't parallelize scans of this table while
> committing this.  Maybe we should go add a sentence about that.
> 

What about

-  the setting of .
+  the setting of .  Setting
this
+  parameter to 0 will disable parallelism for that table.

>> Even if you set a per-table parallel_degree higher than
>> max_parallel_degree, it'll be maxed at max_parallel_degree.
>>
>> Then, the explain shows that the planner assumed it'll launch 9 workers,
>> but only 8 were available (or needed perhaps) at runtime.
> 
> We should probably add the number of workers actually obtained to the
> EXPLAIN ANALYZE output.  That's been requested before.
> 

If it's not too late for 9.6, it would be very great.

>>> postgres=# set max_parallel_degree =262;
>>> ERROR:  262 is outside the valid range for parameter
>>> "max_parallel_degree" (0 .. 262143)
>>>
>>> postgres=# set max_parallel_degree =262143;
>>> SET
>>> postgres=#
>>>
>>> postgres=# explain analyze verbose select * from abd  where n<=1;
>>> ERROR:  requested shared memory size overflows size_t
>>>
>>> if we remove the analyze keyword then query running successfully.
>>>
>>> Expected = Is it not better to throw the error at the time of setting
>>> max_parallel_degree, if not supported ?
>>
>> +1
> 
> It surprises me that that request overflowed size_t.  I guess we
> should look into why that's happening.  Did you test this on a 32-bit
> system?
> 

I can reproduce the same issue on a 64 bits system. Setting
max_parallel_degree to 32768 or above raise this error:

ERROR:  could not resize shared memory segment "/PostgreSQL.44279285" to
18446744072113360072 bytes: Invalid argument

On a 32 bits system, following assert fails:

TRAP: FailedAssertion("!(offset < total_bytes)", File: "shm_toc.c",
Line: 192)

After some gdb, it looks like the overflow comes from

/* Estimate space for tuple queues. */
shm_toc_estimate_chunk(>estimator,

PARALLEL_TUPLE_QUEUE_SIZE * pcxt->nworkers);

372 shm_toc_estimate_chunk(>estimator,
(gdb) p pcxt->estimator
$2 = {space_for_chunks = 3671712, number_of_keys = 3}
(gdb) n
374 shm_toc_estimate_keys(>estimator, 1);
(gdb) p pcxt->estimator
$3 = {space_for_chunks = 18446744071565739680, number_of_keys = 3}


Following change fix the issue:

diff --git a/src/backend/executor/execParallel.c
b/src/backend/executor/execParallel.c
index 572a77b..0a5210e 100644
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -370,7 +370,7 @@ ExecInitParallelPlan(PlanState *planstate, EState
*estate, int nworkers)

/* Estimate space for tuple queues. */
shm_toc_estimate_chunk(>estimator,
-  PARALLEL_TUPLE_QUEUE_SIZE * pcxt->nworkers);
+  (Size) PARALLEL_TUPLE_QUEUE_SIZE *
pcxt->nworkers);
shm_toc_estimate_keys(>estimator, 1);

But the query still fails with "ERROR:  out of shared memory".

-- 
Julien Rouhaud
http://dalibo.com - http://dalibo.org


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

[HACKERS] Removing the TRACE_SORT macro

[Peter Geoghegan]

Currently, debug instrumentation for sorting is only available if the
TRACE_SORT macro was defined when PostgreSQL was compiled. It is
defined by default, and so in practice it's always available; there is
really no upside to disabling it. "18.17. Developer Options" notes
this restriction for trace_sort, which is the only such restriction.

The number of TRACE_SORT elog() logging callers has grown
significantly in the past couple of releases. The associated "#ifdef
TRACE_SORT" crud has also grown.

I propose that we completely remove the TRACE_SORT macro, and all the
associated crud. Just having that as an option suggests that there is
some possible upside to disabling trace_sort, which is clearly not
true. I will write a patch doing this if there are no objections. I
think this is justifiable as clean-up for 9.6.

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

[HACKERS] Remove unused function from walsender.c

[Fujii Masao]

Hi,

There is an unused function GetOldestWALSendPointer() in walsender.c.
Per comment, it was introduced because we may need it in the future for
synchronous replication.

Now we have very similar function SyncRepGetOldestSyncRecPtr() in
syncrep.c. Which makes me think that GetOldestWALSendPointer()
no longer needs to be maintained. So, is it time to remove that unused
function?

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] Lets (not) break all the things. Was: [pgsql-advocacy] 9.6 -> 10.0

[Robert Haas]

On Mon, Apr 11, 2016 at 3:23 PM, Robbie Harwood  wrote:
> I'm sure this won't be a popular suggestion, but in the interest of
> advocating for more cryptography: if we land GSSAPI auth+encryption, I'd
> like the auth-only codepath to go away.

I can't think of a reason that would be a good idea.  Occasionally
good things come from artificially limiting how users can use the
system, but mostly that just annoys people.

-- 
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] Lets (not) break all the things. Was: [pgsql-advocacy] 9.6 -> 10.0

[Robbie Harwood]

Justin Clift  writes:

> Moving over a conversation from the pgsql-advocacy mailing list.  In it
> Simon (CC'd) raised the issue of potentially creating a 
> backwards-compatibility
> breaking release at some point in the future, to deal with things that
> might have no other solution (my wording).
>
> Relevant part of that thread there for reference:
>
>   
> http://www.postgresql.org/message-id/CANP8+jLtk1NtaJyXc=hAqX=0k+ku4zfavgvbkfs+_sor9he...@mail.gmail.com
>
> Simon included a short starter list of potentials which might be in
> that category:
>
>   * SQL compliant identifiers
>   * Remove RULEs
>   * Change recovery.conf
>   * Change block headers
>   * Retire template0, template1
>   * Optimise FSM
>   * Add heap metapage
>   * Alter tuple headers
>   et al
>
> This still is better placed on -hackers though, so lets have the
> conversation here to figure out if a "backwards compatibility breaking"
> release really is needed or not.
>
> Hopefully we can get it all done without giving users a reason to consider
> switching. ;)

I'm sure this won't be a popular suggestion, but in the interest of
advocating for more cryptography: if we land GSSAPI auth+encryption, I'd
like the auth-only codepath to go away.


signature.asc
Description: PGP signature

Re: [HACKERS] Move PinBuffer and UnpinBuffer to atomics

[Andres Freund]

On 2016-04-11 22:08:15 +0300, Alexander Korotkov wrote:
> On Mon, Apr 11, 2016 at 5:04 PM, Alexander Korotkov <
> a.korot...@postgrespro.ru> wrote:
> 
> > On Mon, Apr 11, 2016 at 8:10 AM, Andres Freund  wrote:
> >
> >> Could you retry after applying the attached series of patches?
> >>
> >
> > Yes, I will try with these patches and snapshot too old reverted.
> >
> 
> I've run the same benchmark with 279d86af and 848ef42b reverted.  I've
> tested of all 3 patches from you applied and, for comparison, 3 patches +
> clog buffers reverted back to 32.
> 
> clients patches patches + clog_32
> 1 12594   12556
> 2 26705   26258
> 4 50985   53254
> 8103234  104416
> 10   135321  130893
> 20   268675  267648
> 30   370437  409710
> 40   486512  482382
> 50   539910  525667
> 60   616401  672230
> 70   667864  660853
> 80   924606  737768
> 90  1217435  799581
> 100 1326054  863066
> 110 1446380  980206
> 120 1484920 1000963
> 130 1512440 1058852
> 140 1536181 1088958
> 150 1504750 1134354
> 160 1461513 1132173
> 170 1453943 1158656
> 180 1426288 1120511
> 
> I hardly can understand how clog buffers influence read-only
> benchmark.

My guess is that the number of buffers influences some alignment;
causing a lot of false sharing or something. I.e. the number of clog
buffers itself doesn't play a role, it's just a question of how it
change the memory layout.

> It even harder for me why influence of clog buffers change its
> direction after applying your patches.  But the results are following.
> And I've rechecked some values manually to verify that there is no
> error. > I would be very thankful for any explanation.

Hm. Possibly this patches influenced alignment, but didn't make things
sufficiently stable to guarantee that we're always correctly aligned,
thus the 32bit case now regresses.

Any chance that I could run some tests on that machine myself? It's very
hard to investigate that kind of issue without access; the only thing I
otherwise can do is lob patches at you, till we find the relevant
memory.

If not, one of the things to do is to use perf to compare where cache
misses is happening between the fast and the slow case.

Greetings,

Andres Freund


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

Re: [HACKERS] Move PinBuffer and UnpinBuffer to atomics

[Alexander Korotkov]

On Mon, Apr 11, 2016 at 5:04 PM, Alexander Korotkov <
a.korot...@postgrespro.ru> wrote:

> On Mon, Apr 11, 2016 at 8:10 AM, Andres Freund  wrote:
>
>> Could you retry after applying the attached series of patches?
>>
>
> Yes, I will try with these patches and snapshot too old reverted.
>

I've run the same benchmark with 279d86af and 848ef42b reverted.  I've
tested of all 3 patches from you applied and, for comparison, 3 patches +
clog buffers reverted back to 32.

clients patches patches + clog_32
1 12594   12556
2 26705   26258
4 50985   53254
8103234  104416
10   135321  130893
20   268675  267648
30   370437  409710
40   486512  482382
50   539910  525667
60   616401  672230
70   667864  660853
80   924606  737768
90  1217435  799581
100 1326054  863066
110 1446380  980206
120 1484920 1000963
130 1512440 1058852
140 1536181 1088958
150 1504750 1134354
160 1461513 1132173
170 1453943 1158656
180 1426288 1120511

I hardly can understand how clog buffers influence read-only benchmark.  It
even harder for me why influence of clog buffers change its direction after
applying your patches.  But the results are following.  And I've rechecked
some values manually to verify that there is no error.  I would be very
thankful for any explanation.

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

Re: [HACKERS] PQsendQuery+PQgetResult+PQsetSingleRowMode limitations and support

[Karl O. Pinc]

On Mon, 11 Apr 2016 19:25:20 +0200
"Shulgin, Oleksandr"  wrote:

> On Mon, Apr 11, 2016 at 7:15 PM, Karl O. Pinc  wrote:

> > > Not sure about the part
> > > where you call PQsetSingleRowMode() again after seeing
> > > PGRES_TUPLES_OK: doesn't look to me like you need or want to do
> > > that.  You should only call it immediately after PQsendQuery().  
> >
> > You're quite right.  All but the first PQsetSingleRowMode()
> > calls fail.
> >
> > This seems unfortunate.   What if I submit several SQL statements
> > with one PQsendQuery() call and I only want some of the statements
> > executed in single row mode?  
> 
> I would assume that if you know for which of the statements you want
> the single row mode, then you as well can submit them as separate
> PQsendQuery() calls.

Agreed.  Although I suppose it's possible to know which statements
you want in single row mode but not know how to parse those
statements out of some big string of queries.  Not my problem.  ;-)

> > When the docs here say "query" what they really mean is "set of
> > statements submitted in a single libpq call".  
> 
> Which are the same things more or less, I'm not sure that the extended
> explanation you suggest makes it less confusing.

I'll try to remember to cc-you if and when I send in a doc patch
so you can see if there's any improvement.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] pgsql: Add the "snapshot too old" feature

[Kevin Grittner]

On Mon, Apr 11, 2016 at 8:20 AM, Alexander Korotkov
 wrote:

> 1) We uglify buffer manager interface a lot.  This patch adds 3 more
> arguments to BufferGetPage().  It looks weird.  Should we try to find less
> invasive way for doing this?

As already pointed out, I originally touched about 450 fewer places
in the code, and did not change the signature of BufferGetPage().
I was torn on the argument that we needed a "forced choice" on
checking the snapshot age built into BufferGetPage() -- it is a bit
annoying, but it might prevent a later bug which could silently
return incorrect data.  In the end, I caved to the argument that
the annoyance was worth the improved chances of avoiding such a
bug.

> 2) Did you ever try to examine performance regression?

Yes.  Our customer used big machines for extensive performance
testing -- although they used "paced" input to simulate real users,
and saw nothing but gains.  On my own i7 box, your test scaled to
100 (so it would fit in memory) yielded this:

unpatched:

number of transactions actually processed: 40534737
latency average = 0.739 ms
latency stddev = 0.359 ms
tps = 134973.430374 (including connections establishing)
tps = 135039.395041 (excluding connections establishing)

with the "snapshot too old" patch:

number of transactions actually processed: 40679654
latency average = 0.735 ms
latency stddev = 0.486 ms
tps = 135463.614244 (including connections establishing)
tps = 135866.160933 (excluding connections establishing)

> I tried simple read
> only test on 4x18 Intel server.
> pgbench -s 1000 -c 140 -j 100 -M prepared -S -T 300 -P 1 postgres (data fits
> to shared_buffers)
>
> master-   193 740.3 TPS
> snapshot too old reverted - 1 065 732.6 TPS
>
> So, for read-only benchmark this patch introduces more than 5 times
> regression on big machine.

I did not schedule a saturation test on a big machine.  I guess I
should have done.

I'm confident this can be fixed; your suggestion for a wrapping
"if" is probably sufficient.  I am looking at this and the misuse
of "volatile" now.

Are you able to easily test that or should I book time on one (or
more) of our big machines on my end?

--
Kevin Grittner
EDB: 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] PQsendQuery+PQgetResult+PQsetSingleRowMode limitations and support

[Shulgin, Oleksandr]

On Mon, Apr 11, 2016 at 7:15 PM, Karl O. Pinc  wrote:
>
> Should I submit a regression test or something to ensure
> that this usage is officially supported?  (A grep for
> PQsetSingleRowMode in src/test/ finds no hits.)
> Can I assume because it's documented it'll continue to work?

Pretty much.

> > Not sure about the part
> > where you call PQsetSingleRowMode() again after seeing
> > PGRES_TUPLES_OK: doesn't look to me like you need or want to do
> > that.  You should only call it immediately after PQsendQuery().
>
> You're quite right.  All but the first PQsetSingleRowMode()
> calls fail.
>
> This seems unfortunate.   What if I submit several SQL statements
> with one PQsendQuery() call and I only want some of the statements
> executed in single row mode?

I would assume that if you know for which of the statements you want the
single row mode, then you as well can submit them as separate PQsendQuery()
calls.

> I'm not sure what the use case
> would be but it seems sad that PQsetSingleRowMode() is per
> libpq call and not per sql statement.

It is per query, where query == "argument to PQsendQuery()" :-)

> When the docs here say "query" what they really mean is "set of
> statements submitted in a single libpq call".

Which are the same things more or less, I'm not sure that the extended
explanation you suggest makes it less confusing.

--
Regards,
Alex

Re: [HACKERS] [BUGS] Breakage with VACUUM ANALYSE + partitions

[Andres Freund]

On 2016-04-11 13:04:48 -0400, Robert Haas wrote:
> You're right, but I think that's more because I didn't say it
> correctly than because you haven't done something novel.

Could be.


> DROP and
> relation truncation know about shared buffers, and they go clear
> blocks that that might be affected from it as part of the truncate
> operation, which means that no other backend will see them after they
> are gone.  The lock makes sure that no other references can be added
> while we're busy removing any that are already there.  So I think that
> there is currently an invariant that any block we are attempting to
> access should actually still exist.

Note that we're not actually accessing any blocks, we're just opening a
segment to get the associated file descriptor.


> It sounds like these references are sticking around in backend-private
> memory, which means they are neither protected by locks nor able to be
> cleared out on drop or truncate.  I think that's a new thing, and a
> bit scary.

True. But how would you batch flush requests in a sorted manner
otherwise, without re-opening file descriptors otherwise? And that's
prety essential for performance.

I can think of a number of relatively easy ways to address this:
1) Just zap (or issue?) all pending flush requests when getting an
   smgrinval/smgrclosenode
2) Do 1), but filter for the closed relnode
3) Actually handle the case of the last open segment not being
   RELSEG_SIZE properly in _mdfd_getseg() - mdnblocks() does so.

I'm kind of inclined to do both 3) and 1).

> The possibly-saving grace here, I suppose, is that the references
> we're worried about are just being used to issue hints to the
> operating system.

Indeed.

> So I guess if we sent a hint on a wrong block or
> skip sending a hint altogether because of some failure, no harm done,
> as long as we don't error out.

Which the writeback code is careful not to do; afaics it's just the
"already open segment" issue making problems here.

- Andres


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

Re: [HACKERS] PQsendQuery+PQgetResult+PQsetSingleRowMode limitations and support

[Karl O. Pinc]

On Mon, 11 Apr 2016 15:55:53 +0200
"Shulgin, Oleksandr"  wrote:

> On Fri, Apr 1, 2016 at 7:53 PM, Karl O. Pinc  wrote:
> >
> > On Fri, 1 Apr 2016 05:57:33 +0200
> > "Shulgin, Oleksandr"  wrote:
> >  
> > > On Apr 1, 2016 02:57, "Karl O. Pinc"  wrote:  
> > > >
> > > > I assume there are no questions about supporting a
> > > > similar functionality only without PQsetSingleRowMode,
> > > > as follows:  
> > >
> > > Sorry, but I don't see what is your actual question here?  
> >
> > The question is whether or not the functionality of the first
> > script is supported.  I ask since Bruce was surprised to see
> > this working and questioned whether PG was intended to behave
> > this way.  
> 
> Well, according to the docs it should work, though I don't recall if
> I have really tried that at least once. 

Well, the code does work.  (Mostly, see below.)

Should I submit a regression test or something to ensure
that this usage is officially supported?  (A grep for
PQsetSingleRowMode in src/test/ finds no hits.)
Can I assume because it's documented it'll continue to work?
Where do I go from here?

> Not sure about the part
> where you call PQsetSingleRowMode() again after seeing
> PGRES_TUPLES_OK: doesn't look to me like you need or want to do
> that.  You should only call it immediately after PQsendQuery().

You're quite right.  All but the first PQsetSingleRowMode()
calls fail.

This seems unfortunate.   What if I submit several SQL statements
with one PQsendQuery() call and I only want some of the statements
executed in single row mode?  I'm not sure what the use case
would be but it seems sad that PQsetSingleRowMode() is per
libpq call and not per sql statement.  It seems a little late
to change the API now.  (On the other hand, fewer calls = less
overhead, especially on the network.  So maybe it's just as well
and any deficiencies are best left for future work.)


For the record, here is where I got confused:

I find the docs unclear.  (I've plans to send in a patch, but
I think I'll wait until after finishing as a reviewer for
somebody else's patch.  That is in process now.)

The docs say:

"To enter single-row mode, call PQsetSingleRowMode immediately after a
successful call of PQsendQuery (or a sibling function). This mode
selection is effective only for the currently executing query."
(http://www.postgresql.org/docs/devel/static/libpq-single-row-mode.html)

Now, if the mode selection is effective only for the currently
executing query then if you call PQSetSingleRowMode() only
once after PQsendQuery() then single row mode will only be on
for the first query, when multiple queries are supplied in
the string passed to PQsendQuery().  The other queries won't
be executed in single row mode.

When the docs here say "query" what they really mean is "set of
statements submitted in a single libpq call".


> > Thanks for the clarification.  For some reason I recently
> > got it into my head that the libpq buffering was on the server side,
> > which is really strange since I long ago determined it was
> > client side.  
> 
> There are also a number of cases where the caching will happen on the
> server side: 



> Less obvious is when you have a set-returning-function and use it like
> "SELECT * FROM srffunc()", this will cause the intermediate result to
> be materialized in a tuple store on the server side before it will be
> streamed to the client.  On the other hand, if you use the same
> function as "SELECT srffunc()" you are going to get the same results
> streamed to the client. I've seen this a number of times already and
> I doesn't look like a fundamental limitation of the execution engine
> to me, rather an implementation deficiency.

That is very interesting.  Thanks.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] [BUGS] Breakage with VACUUM ANALYSE + partitions

[Robert Haas]

On Mon, Apr 11, 2016 at 12:50 PM, Andres Freund  wrote:
> On 2016-04-04 12:45:25 -0400, Robert Haas wrote:
>> Well, I agree that it's pretty strange that _mdfd_getseg() makes no
>> such check, but I still don't think I understand what's going on here.
>> Backends shouldn't be requesting nonexistent blocks from a relation -
>> higher-level safeguards, like holding AccessExclusiveLock before
>> trying to complete a DROP or TRUNCATE - are supposed to prevent that.
>
> I don't think that's really true: We write blocks back without holding
> any sort of relation level locks; and thus do _mdfd_getseg() type
> accesses as well.

You're right, but I think that's more because I didn't say it
correctly than because you haven't done something novel.  DROP and
relation truncation know about shared buffers, and they go clear
blocks that that might be affected from it as part of the truncate
operation, which means that no other backend will see them after they
are gone.  The lock makes sure that no other references can be added
while we're busy removing any that are already there.  So I think that
there is currently an invariant that any block we are attempting to
access should actually still exist.  It sounds like these references
are sticking around in backend-private memory, which means they are
neither protected by locks nor able to be cleared out on drop or
truncate.  I think that's a new thing, and a bit scary.

> And we're not really "requesting nonexistant blocks" - the segments are
> just opened to get the associated file descriptor, and they're opened
> with EXTENSION_RETURN_NULL. It turns out to be important
> performance-wise to reuse fd's when triggering kernel writeback.

The possibly-saving grace here, I suppose, is that the references
we're worried about are just being used to issue hints to the
operating system.  So I guess if we sent a hint on a wrong block or
skip sending a hint altogether because of some failure, no harm done,
as long as we don't error out.

-- 
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] [BUGS] Breakage with VACUUM ANALYSE + partitions

[Andres Freund]

On 2016-04-04 12:45:25 -0400, Robert Haas wrote:
> Well, I agree that it's pretty strange that _mdfd_getseg() makes no
> such check, but I still don't think I understand what's going on here.
> Backends shouldn't be requesting nonexistent blocks from a relation -
> higher-level safeguards, like holding AccessExclusiveLock before
> trying to complete a DROP or TRUNCATE - are supposed to prevent that.

I don't think that's really true: We write blocks back without holding
any sort of relation level locks; and thus do _mdfd_getseg() type
accesses as well.

And we're not really "requesting nonexistant blocks" - the segments are
just opened to get the associated file descriptor, and they're opened
with EXTENSION_RETURN_NULL. It turns out to be important
performance-wise to reuse fd's when triggering kernel writeback.

> If this patch is causing us to hold onto smgr references to a relation
> on which we no longer hold locks, I think that's irretrievably broken
> and should be reverted.  I really doubt this will be the only thing
> that goes wrong if you do that.

As we already have done that for writes for a long time, I'm a bit
surprised about that statement.

Greetings,

Andres Freund


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Robert Haas]

On Mon, Apr 11, 2016 at 5:22 AM, Magnus Hagander  wrote:
> Well, if we *don't* do the rewrite before we release it, then we have to
> instead put information about the new version of the functions into the old
> structure I think.

I think that you should have done that in the patch as committed.
Maybe you could take an hour and go do that now, and then you can do
the rewrite when you get to it.

Tracking open issues and getting them resolved is a lot of work, so it
kind of frustrates me that you committed this patch knowing that it
was going to create one when, with only slightly more work, you could
have avoided that.  Perhaps that is rigid and intolerant of me, but if
I had done that for even 25% of the patches I committed this
CommitFest, the open-issues list would be a bloodbath right now.

I apologize if this sounds harsh.  I don't mean to be a jerk.

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


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

[HACKERS] Lets (not) break all the things. Was: [pgsql-advocacy] 9.6 -> 10.0

[Justin Clift]

Moving over a conversation from the pgsql-advocacy mailing list.  In it
Simon (CC'd) raised the issue of potentially creating a backwards-compatibility
breaking release at some point in the future, to deal with things that
might have no other solution (my wording).

Relevant part of that thread there for reference:

  
http://www.postgresql.org/message-id/CANP8+jLtk1NtaJyXc=hAqX=0k+ku4zfavgvbkfs+_sor9he...@mail.gmail.com

Simon included a short starter list of potentials which might be in
that category:

  * SQL compliant identifiers
  * Remove RULEs
  * Change recovery.conf
  * Change block headers
  * Retire template0, template1
  * Optimise FSM
  * Add heap metapage
  * Alter tuple headers
  et al

This still is better placed on -hackers though, so lets have the
conversation here to figure out if a "backwards compatibility breaking"
release really is needed or not.

Hopefully we can get it all done without giving users a reason to consider
switching. ;)

Regards and best wishes,

Justin Clift

--
"My grandfather once told me that there are two kinds of people: those
who work and those who take the credit. He told me to try to be in the
first group; there was less competition there."
- Indira Gandhi



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

[HACKERS] plan for beta1 & open issues

[Robert Haas]

The PostgreSQL 9.6 release management team has determined that
PostgreSQL 9.6beta1 should wrap on May 9, 2016 for release on May
12,2016, subject to the availability of the packaging team to perform
a release at that time.  The release management team believes that
there are no currently-known problems which are both severe enough to
justify postponing beta and complex enough that they cannot be
resolved in advance of that date.  The release management team will
reconsider these dates if new and serious issues are reported which
make it seem imprudent to ship a beta during that week.  This is the
same week during which minor releases for existing back-branches are
scheduled to go out, so this would mean doing 9.6beta1 at the same
time we do back-branches.

The release management team encourages all community members to add
any known issues which should be addressed before the final release of
PostgreSQL 9.6 to
https://wiki.postgresql.org/wiki/PostgreSQL_9.6_Open_Items - community
members who lack edit access should follow the directions at
https://wiki.postgresql.org/wiki/WikiEditing in order to obtain it.
The RMT will determine which of these open items, if any, are beta
blockers; which are issues that need to be addressed before final
release; and which do not need to be addressed for PostgreSQL 9.6. The
RMT does not encourage adding missing features or bugs in
already-released versions to this list; but if there is even a
possibility that an issue might be new in PostgreSQL 9.6, the RMT
encourages community members to be bold in adding that issue to the
list so that it may be properly tracked and considered by the RMT.

Robert Haas
PostgreSQL 9.6 Release Management Team


-- 
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] 'Create table if not exists as' breaks SPI_execute

[Stas Kelvich]

> On 11 Apr 2016, at 18:41, Stas Kelvich  wrote:
> 
> Hi.
> 
> SPI_execute assumes that CreateTableAsStmt always have completionTag == 
> “completionTag”.
> But it isn’t true in case of ‘IF NOT EXISTS’ present.
> 
> 
> 

Sorry, I meant completionTag == “SELECT”.

-- 
Stas Kelvich
Postgres Professional: http://www.postgrespro.com
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] 2016-03 Commitfest

[Robert Haas]

On Mon, Apr 11, 2016 at 8:37 AM, Andreas Karlsson  wrote:
> On 04/11/2016 01:35 PM, David Steele wrote:
>> I've marked this committed so the 2016-03 CF is now complete!
>
> Thanks to you and everyone else involved in running this CF. You did an
> excellent job.

Yeah, David deserves mad props.

-- 
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] Choosing parallel_degree

[Robert Haas]

On Mon, Apr 11, 2016 at 11:27 AM, Julien Rouhaud
 wrote:
> On 11/04/2016 15:56, tushar wrote:
>> On 04/08/2016 08:53 PM, Robert Haas wrote:
>>> On Fri, Apr 8, 2016 at 1:22 AM, Amit Kapila  wrote:
 Other than that, patch looks good and I have marked it as Ready For
 Committer.  Hope, we get this for 9.6.
>>> Committed.  I think this is likely to make parallel query
>>> significantly more usable in 9.6.
>>>
>> While testing ,I observed couple of things -
>>
>> Case 1 =Not accepting parallel seq scan when parallel_degree is set to 0
>>
>> postgres=# create table fok2(n int) with (parallel_degree=0);
>> CREATE TABLE
>> postgres=# insert into fok2 values (generate_series(1,100)); analyze
>> fok2; vacuum fok2;
>> INSERT 0 100
>> ANALYZE
>> VACUUM
>> postgres=# set max_parallel_degree =5;
>> SET
>> postgres=# explain analyze verbose   select * from fok2  where n<=10;
>>   QUERY
>> PLAN
>> --
>>  Seq Scan on public.fok2  (cost=0.00..16925.00 rows=100 width=4) (actual
>> time=0.027..217.882 rows=10 loops=1)
>>Output: n
>>Filter: (fok2.n <= 10)
>>Rows Removed by Filter: 90
>>  Planning time: 0.084 ms
>>  Execution time: 217.935 ms
>> (6 rows)
>>
>> I am assuming parallel_degree=0 is as same as not using it  , i.e
>> create table fok2(n int) with (parallel_degree=0);  = create table
>> fok2(n int);
>>
>> so in this case it should have accepted the parallel seq .scan.
>>
>
> No, setting it to 0 means to force not using parallel workers (but
> considering the parallel path IIRC).

I'm not sure what the parenthesized bit means, because you can't use
parallelism without workers.  But I think I should have made the docs
more clear that 0 = don't parallelize scans of this table while
committing this.  Maybe we should go add a sentence about that.

> Even if you set a per-table parallel_degree higher than
> max_parallel_degree, it'll be maxed at max_parallel_degree.
>
> Then, the explain shows that the planner assumed it'll launch 9 workers,
> but only 8 were available (or needed perhaps) at runtime.

We should probably add the number of workers actually obtained to the
EXPLAIN ANALYZE output.  That's been requested before.

>> postgres=# set max_parallel_degree =262;
>> ERROR:  262 is outside the valid range for parameter
>> "max_parallel_degree" (0 .. 262143)
>>
>> postgres=# set max_parallel_degree =262143;
>> SET
>> postgres=#
>>
>> postgres=# explain analyze verbose select * from abd  where n<=1;
>> ERROR:  requested shared memory size overflows size_t
>>
>> if we remove the analyze keyword then query running successfully.
>>
>> Expected = Is it not better to throw the error at the time of setting
>> max_parallel_degree, if not supported ?
>
> +1

It surprises me that that request overflowed size_t.  I guess we
should look into why that's happening.  Did you test this on a 32-bit
system?

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


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

[HACKERS] 'Create table if not exists as' breaks SPI_execute

[Stas Kelvich]

Hi.

SPI_execute assumes that CreateTableAsStmt always have completionTag == 
“completionTag”.
But it isn’t true in case of ‘IF NOT EXISTS’ present.




spi-cta.patch
Description: Binary data


-- 
Stas Kelvich
Postgres Professional: http://www.postgrespro.com
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] Choosing parallel_degree

[Julien Rouhaud]

On 11/04/2016 15:56, tushar wrote:
> On 04/08/2016 08:53 PM, Robert Haas wrote:
>> On Fri, Apr 8, 2016 at 1:22 AM, Amit Kapila  wrote:
>>> Other than that, patch looks good and I have marked it as Ready For
>>> Committer.  Hope, we get this for 9.6.
>> Committed.  I think this is likely to make parallel query
>> significantly more usable in 9.6.
>>
> While testing ,I observed couple of things -
> 
> Case 1 =Not accepting parallel seq scan when parallel_degree is set to 0
> 
> postgres=# create table fok2(n int) with (parallel_degree=0);
> CREATE TABLE
> postgres=# insert into fok2 values (generate_series(1,100)); analyze
> fok2; vacuum fok2;
> INSERT 0 100
> ANALYZE
> VACUUM
> postgres=# set max_parallel_degree =5;
> SET
> postgres=# explain analyze verbose   select * from fok2  where n<=10;
>   QUERY
> PLAN 
> --
>  Seq Scan on public.fok2  (cost=0.00..16925.00 rows=100 width=4) (actual
> time=0.027..217.882 rows=10 loops=1)
>Output: n
>Filter: (fok2.n <= 10)
>Rows Removed by Filter: 90
>  Planning time: 0.084 ms
>  Execution time: 217.935 ms
> (6 rows)
> 
> I am assuming parallel_degree=0 is as same as not using it  , i.e
> create table fok2(n int) with (parallel_degree=0);  = create table
> fok2(n int);
> 
> so in this case it should have accepted the parallel seq .scan.
> 

No, setting it to 0 means to force not using parallel workers (but
considering the parallel path IIRC).

> Case 2=Total no# of workers are NOT matching with the workers information -
> 
> postgres=# alter table fok set (parallel_degree=10);
> ALTER TABLE
> postgres=# set max_parallel_degree =9;
> SET
> postgres=# explain analyze verbose   select * from fok  where n<=1;
>QUERY
> PLAN   
> -
>  Gather  (cost=1000.00..6823.89 rows=100 width=4) (actual
> time=0.621..107.755 rows=1 loops=1)
>Output: n
> *   Number of Workers: 9*
>->  Parallel Seq Scan on public.fok  (cost=0.00..5814.00 rows=11
> width=4) (actual time=83.382..95.157 rows=0 loops=9)
>  Output: n
>  Filter: (fok.n <= 1)
>  Rows Removed by Filter: 11
>  Worker 0: actual time=82.181..82.181 rows=0 loops=1
>  Worker 1: actual time=97.236..97.236 rows=0 loops=1
>  Worker 2: actual time=93.586..93.586 rows=0 loops=1
>  Worker 3: actual time=94.159..94.159 rows=0 loops=1
>  Worker 4: actual time=88.459..88.459 rows=0 loops=1
>  Worker 5: actual time=90.245..90.245 rows=0 loops=1
>  Worker 6: actual time=101.577..101.577 rows=0 loops=1
>  Worker 7: actual time=102.955..102.955 rows=0 loops=1
>  Planning time: 0.119 ms
>  Execution time: 108.585 ms
> (17 rows)
> 
> Expected = Expecting worker8 information , also loops=10 (including the
> Master)
> 

Even if you set a per-table parallel_degree higher than
max_parallel_degree, it'll be maxed at max_parallel_degree.

Then, the explain shows that the planner assumed it'll launch 9 workers,
but only 8 were available (or needed perhaps) at runtime.

> Case 3=Getting error if we set the max value in max_parallel_degree  as
> well in parallel_degree  .
> 
> postgres=# create table abd(n int) with (parallel_degree=262144);
> ERROR:  value 262144 out of bounds for option "parallel_degree"
> DETAIL:  Valid values are between "0" and "262143".
> 
> postgres=# create table abd(n int) with (parallel_degree=262143);
> CREATE TABLE
> postgres=# insert into abd values (generate_series(1,100)); analyze
> abd; vacuum abd;
> INSERT 0 100
> ANALYZE
> 
> postgres=# set max_parallel_degree =262;
> ERROR:  262 is outside the valid range for parameter
> "max_parallel_degree" (0 .. 262143)
> 
> postgres=# set max_parallel_degree =262143;
> SET
> postgres=#
> 
> postgres=# explain analyze verbose select * from abd  where n<=1;
> ERROR:  requested shared memory size overflows size_t
> 
> if we remove the analyze keyword then query running successfully.
> 
> Expected = Is it not better to throw the error at the time of setting
> max_parallel_degree, if not supported ?

+1


-- 
Julien Rouhaud
http://dalibo.com - http://dalibo.org


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

Re: [HACKERS] [COMMITTERS] pgsql: Add the "snapshot too old" feature

[Alexander Korotkov]

On Mon, Apr 11, 2016 at 4:27 PM, Alvaro Herrera 
wrote:

> Alexander Korotkov wrote:
> > Kevin,
> >
> > This commit makes me very uneasy.  I didn't much care about this patch
> > mainly because I didn't imagine its consequences. Now, I see following:
> >
> > 1) We uglify buffer manager interface a lot.  This patch adds 3 more
> > arguments to BufferGetPage().  It looks weird.  Should we try to find
> less
> > invasive way for doing this?
>
> Kevin's patch was much less invasive originally.  It became invasive in
> the course of later review -- there was fear that future code would
> "forget" the snapshot check accidentally, which would have disastrous
> effects (data becomes invisible without notice).
>

OK, I will read that thread and try to verify these thoughts.


> > 2) Did you ever try to examine performance regression?  I tried simple
> read
> > only test on 4x18 Intel server.
> > pgbench -s 1000 -c 140 -j 100 -M prepared -S -T 300 -P 1 postgres (data
> > fits to shared_buffers)
> >
> > master-   193 740.3 TPS
> > snapshot too old reverted - 1 065 732.6 TPS
> >
> > So, for read-only benchmark this patch introduces more than 5 times
> > regression on big machine.
>
> Wow, this is terrible, but the patch is supposed to have no impact when
> the feature is not in use.  Maybe there's some simple oversight that can
> be fixed.


Perf show that 50% of time is spent in s_lock() called from
GetXLogInsertRecPtr() called from GetSnapshotData().  I think at very least
we should at least surround with "if" checking that "snapshot too old" is
enabled.

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

Re: [HACKERS] Some other things about contrib/bloom and generic_xlog.c

[Tom Lane]

Teodor Sigaev  writes:
>> 2. Unless I'm missing something, contrib/bloom is making no effort
>> to ensure that bloom index pages can be distinguished from other pages
>> by pg_filedump.  That's fine if your expectation is that bloom will always
>> be a toy with no use in production; but otherwise, not so much.  You need
>> to make sure that the last two bytes of the page special space contain a
>> uniquely identifiable code; compare spgist_page_id in SPGiST indexes.

> Now contrib/bloom is a canonical example how to implement yourown index and 
> how 
> to use generic WAL interface. And it is an useful toy: in some cases it could 
> help in production system, patch attached. I'm a little dissatisfied with 
> patch 
> because I had to add unused field to BloomPageOpaqueData, in opposite case 
> size 
> of BloomPageOpaqueData struct equals 6 bytes but special size is MAXALIGNED, 
> so, 
> last two bytes will be unused. If unused field is not a problem, I will push 
> this patch.

Yes, it will mean that the special space will have to be 8 bytes not 4.
But on the other hand, that only makes a difference on 32-bit machines;
if MAXALIGN is 8 then you won't be able to fit anything into those bytes
anyway.  And someday there might be a use for the other two bytes ...
so I'm not particularly concerned by the "wasted space" argument.

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] Execute ignoring cursor?

[Pavel Stehule]

2016-04-11 16:31 GMT+02:00 nummervet nummervet :

> Oh. That doesn't work for me as i generate the query dynamically and don't
> know their structure...
> Maybe there is an easy way to get the cursor structure (column - value,
> column - value)?
> Or should i give up on cursors and try something else? Some Google search
> hint that hstore could be my saviour :)
>

maybe hstore, or json, or C extension - I wrote plpgsql toolbox
https://github.com/okbob/pltoolbox . Another way is using PLPerl,
PLPythonu. PLpgSQL is strongly strict language - it is not designed for
dynamic tasks.

Regards

Pavel


>
> Понедельник, 11 апреля 2016, 16:10 +03:00 от Pavel Stehule <
> pavel.steh...@gmail.com>:
>
>
>
>
> 2016-04-11 13:11 GMT+02:00 nummervet nummervet  >:
>
> Ok, now i am getting this:
> ERROR:  could not identify column "151" in record data type
>
> Raise notice show that the column exists.
> Any other way around it?
>
>
> hmm - it doesn't work for generic record - it should be typed row value.
>
> postgres=# create table foo("123" int);
> CREATE TABLE
>
> postgres=# create table boo("123" int);
> CREATE TABLE
>
> insert into boo values(20);
> INSERT 0 1
>
> postgres=# do $$
> declare r boo; -- cannot be generic record
> begin
>   for r in select * from boo
>   loop
> execute $_$insert into foo values($1."123")$_$ using r;
>   end loop;
> end;
> $$;
> DO
>
> Regards
>
> Pavel
>
>
>
>
> Пятница, 8 апреля 2016, 18:24 +03:00 от Pavel Stehule <
> pavel.steh...@gmail.com
> >:
>
>
>
>
> 2016-04-08 16:46 GMT+02:00 nummervet nummervet  >:
>
> That didn't work for me:
>
> ERROR:  syntax error at or near "$"
> LINE 1: ...ibute_id, set_id ) (select $."151", '...
>
>
> should be $1
>
> Regards
>
> Pavel
>
>
>
>
> Пятница, 8 апреля 2016, 17:25 +03:00 от Pavel Stehule <
> pavel.steh...@gmail.com
> >:
>
>
> Hi
>
> 2016-04-08 16:17 GMT+02:00 nummervet nummervet  >:
>
> Hello. Didn't find dedicated plpgsql list, so decided to post question
> here.
> I am trying to create a  function that will pick up some values from
> cursor and execute them as a dynamic query.
> However, once i use EXECUTE, its seems to be ignoring the existence of
> cursor and try to pick up values from table.
> Basically:
>
> insert into mytable ( value, attribute_id, set_id ) (select rec."151",
> '201', '1')
>
> works, but
>
> execute 'insert into mytable ( value, attribute_id, set_id ) (select
> rec."151", ''201'', ''1'')'
>
>
> Dynamic queries are executed in own space and there are not direct access
> to plpgsql variables.
>
> please, try: execute 'insert into mytable ( value, attribute_id, set_id )
> (select $1."151", ''201'', ''1'')' using rec;
>
> The content should be passed to dynamic query via USING clause.
>
>
> http://www.postgresql.org/docs/current/static/plpgsql-statements.html#PLPGSQL-STATEMENTS-EXECUTING-DYN
>
> Regards
>
> Pavel Stehule
>
>
>
> fails with
>
> ERROR:  missing FROM-clause entry for table "rec"
> LINE 1: ...ibute_id, set_id ) (select rec."151",...
>
> Is there any way around it? Or should i just give up and do it some other
> way?
>
>
>
>
>
>
>
>

[HACKERS] Re[2]: [HACKERS] Execute ignoring cursor?

[nummervet nummervet]

 Oh. That doesn't work for me as i generate the query dynamically and don't 
know their structure...
Maybe there is an easy way to get the cursor structure (column - value, column 
- value)?
Or should i give up on cursors and try something else? Some Google search hint 
that hstore could be my saviour :)

>Понедельник, 11 апреля 2016, 16:10 +03:00 от Pavel Stehule 
>:
>
>
>
>2016-04-11 13:11 GMT+02:00 nummervet nummervet  < nummer...@mail.ru > :
>>Ok, now i am getting this:
>>ERROR:  could not identify column "151" in record data type
>>
>>Raise notice show that the column exists.
>>Any other way around it?
>>
>
>hmm - it doesn't work for generic record - it should be typed row value.
>
>postgres=# create table foo("123" int);
>CREATE TABLE
>
>postgres=# create table boo("123" int);
>CREATE TABLE
>
>insert into boo values(20);
>INSERT 0 1
>
>postgres=# do $$
>declare r boo; -- cannot be generic record
>begin
>  for r in select * from boo
>  loop
>    execute $_$insert into foo values($1."123")$_$ using r;
>  end loop;
>end;
>$$;
>DO
>
>Regards
>
>Pavel
>
> 
>>
>>>Пятница,  8 апреля 2016, 18:24 +03:00 от Pavel Stehule < 
>>>pavel.steh...@gmail.com >:
>>>
>>>
>>>
>>>
>>>2016-04-08 16:46 GMT+02:00 nummervet nummervet  < nummer...@mail.ru > :
That didn't work for me:

ERROR:  syntax error at or near "$"
LINE 1: ...ibute_id, set_id ) (select $."151", '...
>>>
>>>should be $1
>>>
>>>Regards
>>>
>>>Pavel
>>> 


>Пятница,  8 апреля 2016, 17:25 +03:00 от Pavel Stehule < 
>pavel.steh...@gmail.com >:
>
>
>Hi
>
>2016-04-08 16:17 GMT+02:00 nummervet nummervet  < nummer...@mail.ru > :
>>Hello. Didn't find dedicated plpgsql list, so decided to post question 
>>here.
>>I am trying to create a  function that will pick up some values from 
>>cursor and execute them as a dynamic query.
>>However, once i use EXECUTE, its seems to be ignoring the existence of 
>>cursor and try to pick up values from table.
>>Basically:
>>
>>insert into mytable ( value, attribute_id, set_id ) (select rec."151", 
>>'201', '1') 
>>
>>works, but 
>>
>>execute 'insert into mytable ( value, attribute_id, set_id ) (select 
>>rec."151", ''201'', ''1'')'
>
>Dynamic queries are executed in own space and there are not direct access 
>to plpgsql variables. 
>
>please, try: execute 'insert into mytable ( value, attribute_id, set_id ) 
>(select $1."151", ''201'', ''1'')' using rec;
>
>The content should be passed to dynamic query via USING clause.
>
>http://www.postgresql.org/docs/current/static/plpgsql-statements.html#PLPGSQL-STATEMENTS-EXECUTING-DYN
>
>Regards
>
>Pavel Stehule
> 
>>
>>fails with 
>>
>>ERROR:  missing FROM-clause entry for table "rec"
>>LINE 1: ...ibute_id, set_id ) (select rec."151",...
>>
>>Is there any way around it? Or should i just give up and do it some other 
>>way?
>>
>

>>>
>>
>

Re: [HACKERS] Some other things about contrib/bloom and generic_xlog.c

[Tom Lane]

Michael Paquier  writes:
> Actually, as I look at this code for the first time, I find that
> GenericXLogFinish() is a very confusing interface. It makes no
> distinction between a page and the meta data associated to this page,
> this is controlled by a flag isNew and buffer data is saved as some
> delta. Actually, fullmage is not exactly true, this may refer to a
> page that has a hole. It seems to me that we should not have one but
> two routines: GenericXLogRegisterBuffer and
> GenericXLogRegisterBufData, similar to what the normal XLOG routines
> offer.

Hmm.  Maybe the documentation leaves something to be desired, but
I thought that the interface was reasonable if you grant that the
goal is to be easy to use rather than to have maximum performance.
The calling code only has to concern itself with making the actual
changes to the target pages, not with constructing WAL descriptions
of those changes.  Also, the fact that the changes don't have to be
made within a critical section is a bigger help than it might sound.

So I don't really have any problems with the API as such.  I tried
to improve the docs a day or two ago, but maybe that needs further
work.

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] Some other things about contrib/bloom and generic_xlog.c

[Alexander Korotkov]

Hi, Tom!

On Sun, Apr 10, 2016 at 7:49 AM, Tom Lane  wrote:

> 1. It doesn't seem like generic_xlog.c has thought very carefully about
> the semantics of the "hole" between pd_lower and pd_upper.  The mainline
> XLOG code goes to some lengths to ensure that the hole stays all-zeroes;
> for example RestoreBlockImage() explicitly zeroes the hole when restoring
> from a full-page image that has a hole.  But generic_xlog.c's redo routine
> does not do anything comparable, nor does GenericXLogFinish make any
> effort to ensure that the "hole" is all-zeroes after normal application of
> a generic update.  The reason this is of interest is that it means the
> contents of the "hole" could diverge between master and slave, or differ
> between the original state of a database and what it is after a crash and
> recovery.  That would at least complicate forensic comparisons of pages,
> and I think it might also break checksumming.  We thought that this was
> important enough to take the trouble of explicitly zeroing holes during
> mainline XLOG replay.  Shouldn't generic_xlog.c take the same trouble?
>
> 2. Unless I'm missing something, contrib/bloom is making no effort
> to ensure that bloom index pages can be distinguished from other pages
> by pg_filedump.  That's fine if your expectation is that bloom will always
> be a toy with no use in production; but otherwise, not so much.  You need
> to make sure that the last two bytes of the page special space contain a
> uniquely identifiable code; compare spgist_page_id in SPGiST indexes.
>

Thank you for spotting these issues.
I'm going to prepare patches for fixing both of them.

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

Re: [HACKERS] Some other things about contrib/bloom and generic_xlog.c

[Teodor Sigaev]

mainline XLOG replay.  Shouldn't generic_xlog.c take the same trouble?

Yes, it should. Alexander now works on it.



2. Unless I'm missing something, contrib/bloom is making no effort
to ensure that bloom index pages can be distinguished from other pages
by pg_filedump.  That's fine if your expectation is that bloom will always
be a toy with no use in production; but otherwise, not so much.  You need
to make sure that the last two bytes of the page special space contain a
uniquely identifiable code; compare spgist_page_id in SPGiST indexes.


Now contrib/bloom is a canonical example how to implement yourown index and how 
to use generic WAL interface. And it is an useful toy: in some cases it could 
help in production system, patch attached. I'm a little dissatisfied with patch 
because I had to add unused field to BloomPageOpaqueData, in opposite case size 
of BloomPageOpaqueData struct equals 6 bytes but special size is MAXALIGNED, so, 
last two bytes will be unused. If unused field is not a problem, I will push 
this patch.


--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


--
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] Move PinBuffer and UnpinBuffer to atomics

[Alexander Korotkov]

On Mon, Apr 11, 2016 at 8:10 AM, Andres Freund  wrote:

> On 2016-04-10 09:03:37 +0300, Alexander Korotkov wrote:
> > On Sun, Apr 10, 2016 at 8:36 AM, Alexander Korotkov <
> > a.korot...@postgrespro.ru> wrote:
> >
> > > On Sat, Apr 9, 2016 at 10:49 PM, Andres Freund 
> wrote:
> > >
> > >>
> > >>
> > >> On April 9, 2016 12:43:03 PM PDT, Andres Freund 
> > >> wrote:
> > >> >On 2016-04-09 22:38:31 +0300, Alexander Korotkov wrote:
> > >> >> There are results with 5364b357 reverted.
> > >> >
> > >> >Crazy that this has such a negative impact. Amit, can you reproduce
> > >> >that? Alexander, I guess for r/w workload 5364b357 is a benefit on
> that
> > >> >machine as well?
> > >>
> > >> How sure are you about these measurements?
> > >
> > >
> > > I'm pretty sure.  I've retried it multiple times by hand before re-run
> the
> > > script.
> > >
> > >
> > >> Because there really shouldn't be clog lookups one a steady state is
> > >> reached...
> > >>
> > >
> > > Hm... I'm also surprised. There shouldn't be clog lookups once hint
> bits
> > > are set.
> > >
> >
> > I also tried to run perf top during pgbench and get some interesting
> > results.
> >
> > Without 5364b357:
> >5,69%  postgres [.] GetSnapshotData
> >4,47%  postgres [.] LWLockAttemptLock
> >3,81%  postgres [.] _bt_compare
> >3,42%  postgres [.] hash_search_with_hash_value
> >3,08%  postgres [.] LWLockRelease
> >2,49%  postgres [.] PinBuffer.isra.3
> >1,58%  postgres [.] AllocSetAlloc
> >1,17%  [kernel] [k] __schedule
> >1,15%  postgres [.] PostgresMain
> >1,13%  libc-2.17.so [.] vfprintf
> >1,01%  libc-2.17.so [.] __memcpy_ssse3_back
> >
> > With 5364b357:
> >   18,54%  postgres [.] GetSnapshotData
> >3,45%  postgres [.] LWLockRelease
> >3,27%  postgres [.] LWLockAttemptLock
> >3,21%  postgres [.] _bt_compare
> >2,93%  postgres [.] hash_search_with_hash_value
> >2,00%  postgres [.] PinBuffer.isra.3
> >1,32%  postgres [.] AllocSetAlloc
> >1,10%  libc-2.17.so [.] vfprintf
> >
> > Very surprising.  It appears that after 5364b357, GetSnapshotData
> consumes
> > more time.  But I can't see anything depending on clog buffers
> > in GetSnapshotData code...
>
> Could you retry after applying the attached series of patches?
>

Yes, I will try with these patches and snapshot too old reverted.

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

Re: [HACKERS] Move PinBuffer and UnpinBuffer to atomics

[Alexander Korotkov]

On Sun, Apr 10, 2016 at 2:24 PM, Amit Kapila 
wrote:

> On Sun, Apr 10, 2016 at 11:33 AM, Alexander Korotkov <
> a.korot...@postgrespro.ru> wrote:
>
>> On Sun, Apr 10, 2016 at 8:36 AM, Alexander Korotkov <
>> a.korot...@postgrespro.ru> wrote:
>>
>>> On Sat, Apr 9, 2016 at 10:49 PM, Andres Freund 
>>> wrote:
>>>


 On April 9, 2016 12:43:03 PM PDT, Andres Freund 
 wrote:
 >On 2016-04-09 22:38:31 +0300, Alexander Korotkov wrote:
 >> There are results with 5364b357 reverted.
 >
 >Crazy that this has such a negative impact. Amit, can you reproduce
 >that? Alexander, I guess for r/w workload 5364b357 is a benefit on that
 >machine as well?

 How sure are you about these measurements?
>>>
>>>
>>> I'm pretty sure.  I've retried it multiple times by hand before re-run
>>> the script.
>>>
>>>
 Because there really shouldn't be clog lookups one a steady state is
 reached...

>>>
>>> Hm... I'm also surprised. There shouldn't be clog lookups once hint bits
>>> are set.
>>>
>>
>> I also tried to run perf top during pgbench and get some interesting
>> results.
>>
>> Without 5364b357:
>>5,69%  postgres [.] GetSnapshotData
>>4,47%  postgres [.] LWLockAttemptLock
>>3,81%  postgres [.] _bt_compare
>>3,42%  postgres [.] hash_search_with_hash_value
>>3,08%  postgres [.] LWLockRelease
>>2,49%  postgres [.] PinBuffer.isra.3
>>1,58%  postgres [.] AllocSetAlloc
>>1,17%  [kernel] [k] __schedule
>>1,15%  postgres [.] PostgresMain
>>1,13%  libc-2.17.so [.] vfprintf
>>1,01%  libc-2.17.so [.] __memcpy_ssse3_back
>>
>> With 5364b357:
>>   18,54%  postgres [.] GetSnapshotData
>>3,45%  postgres [.] LWLockRelease
>>3,27%  postgres [.] LWLockAttemptLock
>>3,21%  postgres [.] _bt_compare
>>2,93%  postgres [.] hash_search_with_hash_value
>>2,00%  postgres [.] PinBuffer.isra.3
>>1,32%  postgres [.] AllocSetAlloc
>>1,10%  libc-2.17.so [.] vfprintf
>>
>> Very surprising.  It appears that after 5364b357, GetSnapshotData
>> consumes more time.  But I can't see anything depending on clog buffers
>> in GetSnapshotData code...
>>
>
> There is a related fact presented by Mithun C Y as well [1] which suggests
> that Andres's idea of reducing the cost of snapshot shows noticeable gain
> after increasing the clog buffers.  If you read that thread you will notice
> that initially we didn't notice much gain by that idea, but with increased
> clog buffers, it started showing noticeable gain.  If by any chance, you
> can apply that patch and see the results (latest patch is at [2]).
>
>
> [1] -
> http://www.postgresql.org/message-id/CAD__Ouic1Tvnwqm6Wf6j7Cz1Kk1DQgmy0isC7=ogx+3jtfg...@mail.gmail.com
>
> [2] -
> http://www.postgresql.org/message-id/cad__ouiwei5she2wwqck36ac9qmhvjuqg3cfpn+ofcmb7rd...@mail.gmail.com
>

I took a look at this thread but I still didn't get why number of clog
buffers affects read-only benchmark.
Could you please explain it to me in more details?

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

Re: [HACKERS] PQsendQuery+PQgetResult+PQsetSingleRowMode limitations and support

[Shulgin, Oleksandr]

On Fri, Apr 1, 2016 at 7:53 PM, Karl O. Pinc  wrote:
>
> On Fri, 1 Apr 2016 05:57:33 +0200
> "Shulgin, Oleksandr"  wrote:
>
> > On Apr 1, 2016 02:57, "Karl O. Pinc"  wrote:
> > >
> > > I assume there are no questions about supporting a
> > > similar functionality only without PQsetSingleRowMode,
> > > as follows:
> >
> > Sorry, but I don't see what is your actual question here?
>
> The question is whether or not the functionality of the first
> script is supported.  I ask since Bruce was surprised to see
> this working and questioned whether PG was intended to behave
> this way.

Well, according to the docs it should work, though I don't recall if I have
really tried that at least once.  Not sure about the part where you call
PQsetSingleRowMode() again after seeing PGRES_TUPLES_OK: doesn't look to me
like you need or want to do that.  You should only call it immediately
after PQsendQuery().

> > Both code examples are going to compile and work, AFAICS. The
> > difference is that the latter will try to fetch the whole result set
> > into client's memory before returning you a PGresult.
>
> Thanks for the clarification.  For some reason I recently
> got it into my head that the libpq buffering was on the server side,
> which is really strange since I long ago determined it was
> client side.

There are also a number of cases where the caching will happen on the
server side: using ORDER BY without an index available to fetch the records
in the required order is the most obvious one.

Less obvious is when you have a set-returning-function and use it like
"SELECT * FROM srffunc()", this will cause the intermediate result to be
materialized in a tuple store on the server side before it will be streamed
to the client.  On the other hand, if you use the same function as "SELECT
srffunc()" you are going to get the same results streamed to the client.
I've seen this a number of times already and I doesn't look like a
fundamental limitation of the execution engine to me, rather an
implementation deficiency.

Another plausible approach to get the results row by row is invoking COPY
protocol with the query: "COPY (SELECT ...) TO STDOUT".  This way you lose
the type information of course, but it still might be appropriate for some
use cases.

--
Regards,
Alex

Re: [HACKERS] Choosing parallel_degree

[tushar]

On 04/08/2016 08:53 PM, Robert Haas wrote:

On Fri, Apr 8, 2016 at 1:22 AM, Amit Kapila  wrote:

Other than that, patch looks good and I have marked it as Ready For
Committer.  Hope, we get this for 9.6.

Committed.  I think this is likely to make parallel query
significantly more usable in 9.6.


While testing ,I observed couple of things -

Case 1 =Not accepting parallel seq scan when parallel_degree is set to 0

postgres=# create table fok2(n int) with (parallel_degree=0);
CREATE TABLE
postgres=# insert into fok2 values (generate_series(1,100)); analyze 
fok2; vacuum fok2;

INSERT 0 100
ANALYZE
VACUUM
postgres=# set max_parallel_degree =5;
SET
postgres=# explain analyze verbose   select * from fok2  where n<=10;
  QUERY PLAN
--
 Seq Scan on public.fok2  (cost=0.00..16925.00 rows=100 width=4) 
(actual time=0.027..217.882 rows=10 loops=1)

   Output: n
   Filter: (fok2.n <= 10)
   Rows Removed by Filter: 90
 Planning time: 0.084 ms
 Execution time: 217.935 ms
(6 rows)

I am assuming parallel_degree=0 is as same as not using it  , i.e
create table fok2(n int) with (parallel_degree=0);  = create table 
fok2(n int);


so in this case it should have accepted the parallel seq .scan.

Case 2=Total no# of workers are NOT matching with the workers information -

postgres=# alter table fok set (parallel_degree=10);
ALTER TABLE
postgres=# set max_parallel_degree =9;
SET
postgres=# explain analyze verbose   select * from fok  where n<=1;
   QUERY PLAN
-
 Gather  (cost=1000.00..6823.89 rows=100 width=4) (actual 
time=0.621..107.755 rows=1 loops=1)

   Output: n
*   Number of Workers: 9*
   ->  Parallel Seq Scan on public.fok  (cost=0.00..5814.00 rows=11 
width=4) (actual time=83.382..95.157 rows=0 loops=9)

 Output: n
 Filter: (fok.n <= 1)
 Rows Removed by Filter: 11
 Worker 0: actual time=82.181..82.181 rows=0 loops=1
 Worker 1: actual time=97.236..97.236 rows=0 loops=1
 Worker 2: actual time=93.586..93.586 rows=0 loops=1
 Worker 3: actual time=94.159..94.159 rows=0 loops=1
 Worker 4: actual time=88.459..88.459 rows=0 loops=1
 Worker 5: actual time=90.245..90.245 rows=0 loops=1
 Worker 6: actual time=101.577..101.577 rows=0 loops=1
 Worker 7: actual time=102.955..102.955 rows=0 loops=1
 Planning time: 0.119 ms
 Execution time: 108.585 ms
(17 rows)

Expected = Expecting worker8 information , also loops=10 (including the 
Master)


Case 3=Getting error if we set the max value in max_parallel_degree as 
well in parallel_degree  .


postgres=# create table abd(n int) with (parallel_degree=262144);
ERROR:  value 262144 out of bounds for option "parallel_degree"
DETAIL:  Valid values are between "0" and "262143".

postgres=# create table abd(n int) with (parallel_degree=262143);
CREATE TABLE
postgres=# insert into abd values (generate_series(1,100)); analyze 
abd; vacuum abd;

INSERT 0 100
ANALYZE

postgres=# set max_parallel_degree =262;
ERROR:  262 is outside the valid range for parameter 
"max_parallel_degree" (0 .. 262143)


postgres=# set max_parallel_degree =262143;
SET
postgres=#

postgres=# explain analyze verbose select * from abd  where n<=1;
ERROR:  requested shared memory size overflows size_t

if we remove the analyze keyword then query running successfully.

Expected = Is it not better to throw the error at the time of setting 
max_parallel_degree, if not supported ?


--
regards,tushar

[HACKERS] Preprocessor condition fix

[Christian Ullrich]

Here is a one-line patch to fix a wrong preprocessor condition in 
pg_regress, found because the VS 2015 compiler warns on the cast in the 
32-bit branch where apparently earlier versions did not.


According to git grep, this is the only place where WIN64 is used 
without the leading underscore.


--
Christian
From 9bc9e8ed79747f7bf3e727c9f64f4a088de589fb Mon Sep 17 00:00:00 2001
From: Christian Ullrich 
Date: Mon, 11 Apr 2016 15:47:20 +0200
Subject: [PATCH] Fixed preprocessor condition (WIN64 -> _WIN64).

---
 src/test/regress/pg_regress.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 1674445..2f6f56d 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -2386,7 +2386,7 @@ regression_main(int argc, char *argv[], init_function 
ifunc, test_function tfunc
 
postmaster_running = true;
 
-#ifdef WIN64
+#ifdef _WIN64
 /* need a series of two casts to convert HANDLE without compiler warning */
 #define ULONGPID(x) (unsigned long) (unsigned long long) (x)
 #else
-- 
2.7.0.windows.1


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

Re: [HACKERS] Relax requirement for INTO with SELECT in pl/pgsql

[Merlin Moncure]

On Sun, Apr 10, 2016 at 3:13 AM, Pavel Stehule  wrote:
>
> Hi
>
> 2016-03-21 22:13 GMT+01:00 Pavel Stehule :
>>
>> Hi
>>
>> 2016-03-21 21:24 GMT+01:00 Merlin Moncure :
>>>
>>> Patch is trivial (see below), discussion is not :-).
>>>
>>> I see no useful reason to require INTO when returning data with
>>> SELECT.  However, requiring queries to indicate not needing data via
>>> PERFORM causes some annoyances:
>>>
>>> *) converting routines back and forth between pl/pgsql and pl/sql
>>> requires needless busywork and tends to cause errors to be thrown at
>>> runtime
>>>
>>> *) as much as possible, (keywords begin/end remain a problem),
>>> pl/pgsql should be a superset of sql
>>>
>>> *) it's much more likely to be burned by accidentally forgetting to
>>> swap in PERFORM than to accidentally leave in a statement with no
>>> actionable target.  Even if you did so in the latter case, it stands
>>> to reason you'd accidentally leave in the target variable, too.
>>>
>>> *) the PERFORM requirement hails from the days when only statements
>>> starting with SELECT return data.  There is no PERFORM equivalent for
>>> WITH/INSERT/DELETE/UPDATE and there are real world scenarios where you
>>> might have a RETURNING clause that does something but not necessarily
>>> want to place the result in a variable (for example passing to
>>> volatile function).  Take a look at the errhint() clause below -- we
>>> don't even have a suggestion in that case.
>>>
>>> This has come up before, and there was a fair amount of sympathy for
>>> this argument albeit with some dissent -- notably Pavel.  I'd like to
>>> get a hearing on the issue -- thanks.  If we decide to move forward,
>>> this would effectively deprecate PERFORM and the documentation will be
>>> suitably modified as well.
>>
>>
>
> here is another argument why this idea is not good.
>
> http://stackoverflow.com/questions/36509511/error-query-has-no-destination-for-result-data-when-writing-pl-pgsql-function
>
> Now, when people coming from T-SQL world use some T-SQL constructs, then 
> usually the code should not work with the error "query has not destination 
> for data ... "
>
> When PLpgSQL will be more tolerant, then their code will be executed without 
> any error, but will not work.

I don't think it's a problem requiring people to use RETURN in order
to return data from the function.

SQL functions BTW happily discard results and it's never been an issue
there FWICT.  To address your other argument given below, there are
valid cases where you'd use RETURNING without having any interest in
capturing the set.  For example, you might have a volatile function,
v_func() that does something and returns a value that may not be
essential to the caller (say, a count of rows adjusted).

INSERT INTO foo ...
RETURNING v_func(foo.x);

Scenarios (even if not very common) where dummy variables are required
and/or queries have to be written into more complex forms (say, into a
CTE) where you would not have to do so outside pl/pgsql greatly
outweigh your points that, 'users might do the wrong thing'.  The
wrong thing is actually the right thing in some cases.

Small aside here: One thing that t-sql did right and pl/sql did wrong
was to make the language a proper superset of sql.  pl/pgsql's
hijacking INTO, BEGIN, END, and EXECUTE are really unfortunate as are
any behaviors that are incompatible with the regular language (like
requiring PERFORM); they fork the language and make building stored
procedures in pl/pgsql much more difficult if not impossible.  I'm not
sure this is a really solvable problem, but at least it can be nibbled
at.

What are the rules for pl/psm?

merlin


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

Re: [HACKERS] [COMMITTERS] pgsql: Add the "snapshot too old" feature

[Alvaro Herrera]

Alexander Korotkov wrote:
> Kevin,
> 
> This commit makes me very uneasy.  I didn't much care about this patch
> mainly because I didn't imagine its consequences. Now, I see following:
> 
> 1) We uglify buffer manager interface a lot.  This patch adds 3 more
> arguments to BufferGetPage().  It looks weird.  Should we try to find less
> invasive way for doing this?

Kevin's patch was much less invasive originally.  It became invasive in
the course of later review -- there was fear that future code would
"forget" the snapshot check accidentally, which would have disastrous
effects (data becomes invisible without notice).

> 2) Did you ever try to examine performance regression?  I tried simple read
> only test on 4x18 Intel server.
> pgbench -s 1000 -c 140 -j 100 -M prepared -S -T 300 -P 1 postgres (data
> fits to shared_buffers)
> 
> master-   193 740.3 TPS
> snapshot too old reverted - 1 065 732.6 TPS
> 
> So, for read-only benchmark this patch introduces more than 5 times
> regression on big machine.

Wow, this is terrible, but the patch is supposed to have no impact when
the feature is not in use.  Maybe there's some simple oversight that can
be fixed.

-- 
Á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] [COMMITTERS] pgsql: Add the "snapshot too old" feature

[Alexander Korotkov]

Kevin,

This commit makes me very uneasy.  I didn't much care about this patch
mainly because I didn't imagine its consequences. Now, I see following:

1) We uglify buffer manager interface a lot.  This patch adds 3 more
arguments to BufferGetPage().  It looks weird.  Should we try to find less
invasive way for doing this?
2) Did you ever try to examine performance regression?  I tried simple read
only test on 4x18 Intel server.
pgbench -s 1000 -c 140 -j 100 -M prepared -S -T 300 -P 1 postgres (data
fits to shared_buffers)

master-   193 740.3 TPS
snapshot too old reverted - 1 065 732.6 TPS

So, for read-only benchmark this patch introduces more than 5 times
regression on big machine.

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

Re: [HACKERS] Execute ignoring cursor?

[Pavel Stehule]

2016-04-11 13:11 GMT+02:00 nummervet nummervet :

> Ok, now i am getting this:
> ERROR:  could not identify column "151" in record data type
>
> Raise notice show that the column exists.
> Any other way around it?
>
>
hmm - it doesn't work for generic record - it should be typed row value.

postgres=# create table foo("123" int);
CREATE TABLE

postgres=# create table boo("123" int);
CREATE TABLE

insert into boo values(20);
INSERT 0 1

postgres=# do $$
declare r boo; -- cannot be generic record
begin
  for r in select * from boo
  loop
execute $_$insert into foo values($1."123")$_$ using r;
  end loop;
end;
$$;
DO

Regards

Pavel



>
> Пятница, 8 апреля 2016, 18:24 +03:00 от Pavel Stehule <
> pavel.steh...@gmail.com>:
>
>
>
>
> 2016-04-08 16:46 GMT+02:00 nummervet nummervet  >:
>
> That didn't work for me:
>
> ERROR:  syntax error at or near "$"
> LINE 1: ...ibute_id, set_id ) (select $."151", '...
>
>
> should be $1
>
> Regards
>
> Pavel
>
>
>
>
> Пятница, 8 апреля 2016, 17:25 +03:00 от Pavel Stehule <
> pavel.steh...@gmail.com
> >:
>
>
> Hi
>
> 2016-04-08 16:17 GMT+02:00 nummervet nummervet  >:
>
> Hello. Didn't find dedicated plpgsql list, so decided to post question
> here.
> I am trying to create a  function that will pick up some values from
> cursor and execute them as a dynamic query.
> However, once i use EXECUTE, its seems to be ignoring the existence of
> cursor and try to pick up values from table.
> Basically:
>
> insert into mytable ( value, attribute_id, set_id ) (select rec."151",
> '201', '1')
>
> works, but
>
> execute 'insert into mytable ( value, attribute_id, set_id ) (select
> rec."151", ''201'', ''1'')'
>
>
> Dynamic queries are executed in own space and there are not direct access
> to plpgsql variables.
>
> please, try: execute 'insert into mytable ( value, attribute_id, set_id )
> (select $1."151", ''201'', ''1'')' using rec;
>
> The content should be passed to dynamic query via USING clause.
>
>
> http://www.postgresql.org/docs/current/static/plpgsql-statements.html#PLPGSQL-STATEMENTS-EXECUTING-DYN
>
> Regards
>
> Pavel Stehule
>
>
>
> fails with
>
> ERROR:  missing FROM-clause entry for table "rec"
> LINE 1: ...ibute_id, set_id ) (select rec."151",...
>
> Is there any way around it? Or should i just give up and do it some other
> way?
>
>
>
>
>
>

Re: [HACKERS] 2016-03 Commitfest

[Andreas Karlsson]

On 04/11/2016 01:35 PM, David Steele wrote:

I've marked this committed so the 2016-03 CF is now complete!


Thanks to you and everyone else involved in running this CF. You did an 
excellent job.


Andreas


--
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] 2016-03 Commitfest

[Magnus Hagander]

On Mon, Apr 11, 2016 at 1:35 PM, David Steele  wrote:

> On 4/10/16 11:14 PM, Andres Freund wrote:
>
> > On 2016-04-08 11:52:45 -0400, Robert Haas wrote:
> >
> >> In view of these circumstances, the RMT has voted to extend the
> >> deadline for this particular patch by 2.5 days; that is, this patch
> >> may be committed with RMT approval no later than 2016-04-11 12:00:00
> >> GMT, which I believe is approximately 4am Monday morning where you
> >> are.
> >
> > I've pushed this now. Didn't find anything really grave; fixed some easy
> > to misunderstand variable naming, and some variables declared again in a
> > nested scope (correct, but confusing).  Thanks for the extension.
>
> I've marked this committed so the 2016-03 CF is now complete!
>
>
Good job!

I've then marked the CF itself as closed as well.

Next step, open items list :)


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

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

[Fujii Masao]

On Mon, Apr 11, 2016 at 5:52 PM, Masahiko Sawada  wrote:
> On Mon, Apr 11, 2016 at 1:31 PM, Fujii Masao  wrote:
>> On Mon, Apr 11, 2016 at 10:58 AM, Masahiko Sawada  
>> wrote:
>>> On Sat, Apr 9, 2016 at 12:32 PM, Tom Lane  wrote:
 Jeff Janes  writes:
> When I compile now without cassert, I get the compiler warning:

> syncrep.c: In function 'SyncRepUpdateConfig':
> syncrep.c:878:6: warning: variable 'parse_rc' set but not used
> [-Wunused-but-set-variable]

 If there's a good reason for that to be an Assert, I don't see it.
 There are no callers of SyncRepUpdateConfig that look like they
 need to, or should expect not to have to, tolerate errors.
 I think the way to fix this is to turn the Assert into a plain
 old test-and-ereport-ERROR.

>>>
>>> I've changed the draft patch Amit implemented so that it doesn't parse
>>> twice(check_hook and assign_hook).
>>> So assertion that was in assign_hook is no longer necessary.
>>>
>>> Please find attached.
>>
>> Thanks for the patch!
>>
>> When I emptied s_s_names, reloaded the configration file, set it to 
>> 'standby1'
>> and reloaded the configuration file again, the master crashed with
>> the following error.
>>
>> *** glibc detected *** postgres: wal sender process postgres [local]
>> streaming 0/3015F18: munmap_chunk(): invalid pointer:
>> 0x024d9a40 ***
>> === Backtrace: =
>> *** glibc detected *** postgres: wal sender process postgres [local]
>> streaming 0/3015F18: munmap_chunk(): invalid pointer:
>> 0x024d9a40 ***
>> /lib64/libc.so.6[0x3be8e75f4e]
>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x97dae2]
>> === Backtrace: =
>> /lib64/libc.so.6[0x3be8e75f4e]
>> postgres: wal sender process postgres [local] streaming
>> 0/3015F18(set_config_option+0x12cb)[0x982242]
>> postgres: wal sender process postgres [local] streaming
>> 0/3015F18(SetConfigOption+0x4b)[0x9827ff]
>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x97dae2]
>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x988b4e]
>> postgres: wal sender process postgres [local] streaming
>> 0/3015F18(set_config_option+0x12cb)[0x982242]
>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x98af40]
>> postgres: wal sender process postgres [local] streaming
>> 0/3015F18(SetConfigOption+0x4b)[0x9827ff]
>> postgres: wal sender process postgres [local] streaming
>> 0/3015F18(ProcessConfigFile+0x9f)[0x98a98b]
>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x988b4e]
>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x98af40]
>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7b50fd]
>> postgres: wal sender process postgres [local] streaming
>> 0/3015F18(ProcessConfigFile+0x9f)[0x98a98b]
>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7b359c]
>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7b50fd]
>> postgres: wal sender process postgres [local] streaming
>> 0/3015F18(exec_replication_command+0x1a7)[0x7b47b6]
>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7b359c]
>> postgres: wal sender process postgres [local] streaming
>> 0/3015F18(PostgresMain+0x772)[0x8141b6]
>> postgres: wal sender process postgres [local] streaming
>> 0/3015F18(exec_replication_command+0x1a7)[0x7b47b6]
>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7896f7]
>> postgres: wal sender process postgres [local] streaming
>> 0/3015F18(PostgresMain+0x772)[0x8141b6]
>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x788e62]
>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7896f7]
>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x785544]
>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x788e62]
>> postgres: wal sender process postgres [local] streaming
>> 0/3015F18(PostmasterMain+0x1134)[0x784c08]
>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x785544]
>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x6ce12e]
>> /lib64/libc.so.6(__libc_start_main+0xfd)[0x3be8e1ed5d]
>> postgres: wal sender process postgres [local] streaming
>> 0/3015F18(PostmasterMain+0x1134)[0x784c08]
>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x467e99]
>>
>
> Thank you for reviewing.
>
> SyncRepUpdateConfig() seems to be no longer necessary.

Really? I was thinking that something like that function needs to
be called at the beginning of a backend and walsender in
EXEC_BACKEND case. No?

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] 2016-03 Commitfest

[David Steele]

On 4/10/16 11:14 PM, Andres Freund wrote:

> On 2016-04-08 11:52:45 -0400, Robert Haas wrote:
>
>> In view of these circumstances, the RMT has voted to extend the
>> deadline for this particular patch by 2.5 days; that is, this patch
>> may be committed with RMT approval no later than 2016-04-11 12:00:00
>> GMT, which I believe is approximately 4am Monday morning where you
>> are.
> 
> I've pushed this now. Didn't find anything really grave; fixed some easy
> to misunderstand variable naming, and some variables declared again in a
> nested scope (correct, but confusing).  Thanks for the extension.

I've marked this committed so the 2016-03 CF is now complete!

-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] [COMMITTERS] pgsql: Move each SLRU's lwlocks to a separate tranche.

[Andres Freund]

Hi,

On 2016-04-11 07:09:18 -0400, Robert Haas wrote:
> Attached patch does that.

Thanks.


> > Additionally, doesn't this obsolete
> >
> > /*
> >  * Preferred alignment for disk I/O buffers.  On some CPUs, copies between
> >  * user space and kernel space are significantly faster if the user buffer
> >  * is aligned on a larger-than-MAXALIGN boundary.  Ideally this should be
> >  * a platform-dependent value, but for now we just hard-wire it.
> >  */
> > #define ALIGNOF_BUFFER  32
> 
> I didn't go as far as trying to remove this; a few other random things
> are using it.

All of the places using it look like they actually rather should use
CACHELINEALIGN...


> diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c
> index 1ad68cd..8e6cbdb 100644
> --- a/src/backend/storage/ipc/shmem.c
> +++ b/src/backend/storage/ipc/shmem.c
> @@ -112,6 +112,7 @@ void
>  InitShmemAllocation(void)
>  {
>   PGShmemHeader *shmhdr = ShmemSegHdr;
> + char   *aligned;
>  
>   Assert(shmhdr != NULL);
>  
> @@ -139,6 +140,11 @@ InitShmemAllocation(void)
>   shmhdr->freeoffset += MAXALIGN(sizeof(slock_t));
>   Assert(shmhdr->freeoffset <= shmhdr->totalsize);
>  
> + /* Make sure the first allocation begins on a cache line boundary. */
> + aligned = (char *)
> + (CACHELINEALIGNchar *) shmhdr) + shmhdr->freeoffset)));
> + shmhdr->freeoffset = aligned - (char *) shmhdr;
> +
>   SpinLockInit(ShmemLock);

In the patch attached to 
http://www.postgresql.org/message-id/20160411051004.yvniqb2pkc7re...@alap3.anarazel.de
I did this instead by fixing up the actual shared memory allocation,
which seems a tiny bit cleaner.  But the asserts added there seem like a
worthwhile addition to me.

Greetings,

Andres Freund


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

[HACKERS] Re[4]: [HACKERS] Execute ignoring cursor?

[nummervet nummervet]

 Ok, now i am getting this:
ERROR:  could not identify column "151" in record data type

Raise notice show that the column exists.
Any other way around it?


>Пятница,  8 апреля 2016, 18:24 +03:00 от Pavel Stehule 
>:
>
>
>
>2016-04-08 16:46 GMT+02:00 nummervet nummervet  < nummer...@mail.ru > :
>>That didn't work for me:
>>
>>ERROR:  syntax error at or near "$"
>>LINE 1: ...ibute_id, set_id ) (select $."151", '...
>
>should be $1
>
>Regards
>
>Pavel
> 
>>
>>
>>>Пятница,  8 апреля 2016, 17:25 +03:00 от Pavel Stehule < 
>>>pavel.steh...@gmail.com >:
>>>
>>>
>>>Hi
>>>
>>>2016-04-08 16:17 GMT+02:00 nummervet nummervet  < nummer...@mail.ru > :
Hello. Didn't find dedicated plpgsql list, so decided to post question here.
I am trying to create a  function that will pick up some values from cursor 
and execute them as a dynamic query.
However, once i use EXECUTE, its seems to be ignoring the existence of 
cursor and try to pick up values from table.
Basically:

insert into mytable ( value, attribute_id, set_id ) (select rec."151", 
'201', '1') 

works, but 

execute 'insert into mytable ( value, attribute_id, set_id ) (select 
rec."151", ''201'', ''1'')'
>>>
>>>Dynamic queries are executed in own space and there are not direct access to 
>>>plpgsql variables. 
>>>
>>>please, try: execute 'insert into mytable ( value, attribute_id, set_id ) 
>>>(select $1."151", ''201'', ''1'')' using rec;
>>>
>>>The content should be passed to dynamic query via USING clause.
>>>
>>>http://www.postgresql.org/docs/current/static/plpgsql-statements.html#PLPGSQL-STATEMENTS-EXECUTING-DYN
>>>
>>>Regards
>>>
>>>Pavel Stehule
>>> 

fails with 

ERROR:  missing FROM-clause entry for table "rec"
LINE 1: ...ibute_id, set_id ) (select rec."151",...

Is there any way around it? Or should i just give up and do it some other 
way?

>>>
>>
>

Re: [HACKERS] [COMMITTERS] pgsql: Move each SLRU's lwlocks to a separate tranche.

[Robert Haas]

On Mon, Apr 11, 2016 at 12:01 AM, Andres Freund  wrote:
>> InitBufferPool() manually fixes up alignment; that should probably be
>> removed now.

Attached patch does that.

>> I also wonder if it doesn't make sense to fix PG_CACHE_LINE_SIZE to
>> 64byte on x86. I personally think a manual ifdef in pg_config_manual.h
>> is ok for that.
>
> Also, this doesn't seem to be complete. This now aligns sizes to
> cacheline boundaries, but it doesn't actually align the returned values
> afaics? That might kind of work sometimes, if freeoffset is initially
> aligned to PG_CACHE_LINE_SIZE, but that's not guaranteed, it's just
> shmhdr->freeoffset += MAXALIGN(sizeof(slock_t));

And tries to fix that.

> Additionally, doesn't this obsolete
>
> /*
>  * Preferred alignment for disk I/O buffers.  On some CPUs, copies between
>  * user space and kernel space are significantly faster if the user buffer
>  * is aligned on a larger-than-MAXALIGN boundary.  Ideally this should be
>  * a platform-dependent value, but for now we just hard-wire it.
>  */
> #define ALIGNOF_BUFFER  32

I didn't go as far as trying to remove this; a few other random things
are using it.

> and
>
> /* extra alignment for large requests, since they are probably 
> buffers */
> if (size >= BLCKSZ)
> newStart = BUFFERALIGN(newStart);

But I got this one.

I fixed the InitBufferPool issue you mentioned in the other email, too.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/storage/buffer/buf_init.c b/src/backend/storage/buffer/buf_init.c
index a5cffc7..5804870 100644
--- a/src/backend/storage/buffer/buf_init.c
+++ b/src/backend/storage/buffer/buf_init.c
@@ -76,11 +76,9 @@ InitBufferPool(void)
 
 	/* Align descriptors to a cacheline boundary. */
 	BufferDescriptors = (BufferDescPadded *)
-		CACHELINEALIGN(
-	   ShmemInitStruct("Buffer Descriptors",
-	   NBuffers * sizeof(BufferDescPadded)
-	   + PG_CACHE_LINE_SIZE,
-	   ));
+		ShmemInitStruct("Buffer Descriptors",
+		NBuffers * sizeof(BufferDescPadded),
+		);
 
 	BufferBlocks = (char *)
 		ShmemInitStruct("Buffer Blocks",
@@ -88,10 +86,9 @@ InitBufferPool(void)
 
 	/* Align lwlocks to cacheline boundary */
 	BufferIOLWLockArray = (LWLockMinimallyPadded *)
-		CACHELINEALIGN(ShmemInitStruct("Buffer IO Locks",
-			  NBuffers * (Size) sizeof(LWLockMinimallyPadded)
-	   + PG_CACHE_LINE_SIZE,
-	   ));
+		ShmemInitStruct("Buffer IO Locks",
+		NBuffers * (Size) sizeof(LWLockMinimallyPadded),
+		);
 
 	BufferIOLWLockTranche.name = "buffer_io";
 	BufferIOLWLockTranche.array_base = BufferIOLWLockArray;
diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c
index 1ad68cd..8e6cbdb 100644
--- a/src/backend/storage/ipc/shmem.c
+++ b/src/backend/storage/ipc/shmem.c
@@ -112,6 +112,7 @@ void
 InitShmemAllocation(void)
 {
 	PGShmemHeader *shmhdr = ShmemSegHdr;
+	char	   *aligned;
 
 	Assert(shmhdr != NULL);
 
@@ -139,6 +140,11 @@ InitShmemAllocation(void)
 	shmhdr->freeoffset += MAXALIGN(sizeof(slock_t));
 	Assert(shmhdr->freeoffset <= shmhdr->totalsize);
 
+	/* Make sure the first allocation begins on a cache line boundary. */
+	aligned = (char *)
+		(CACHELINEALIGNchar *) shmhdr) + shmhdr->freeoffset)));
+	shmhdr->freeoffset = aligned - (char *) shmhdr;
+
 	SpinLockInit(ShmemLock);
 
 	/* ShmemIndex can't be set up yet (need LWLocks first) */
@@ -189,10 +195,6 @@ ShmemAlloc(Size size)
 
 	newStart = ShmemSegHdr->freeoffset;
 
-	/* extra alignment for large requests, since they are probably buffers */
-	if (size >= BLCKSZ)
-		newStart = BUFFERALIGN(newStart);
-
 	newFree = newStart + size;
 	if (newFree <= ShmemSegHdr->totalsize)
 	{

-- 
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] Speedup twophase transactions

[Stas Kelvich]

> On 08 Apr 2016, at 16:09, Stas Kelvich  wrote:
> 
> Tried on linux and os x 10.11 and 10.4.
> 
> Still can’t reproduce, but have played around with your backtrace.
> 
> I see in xlodump on slave following sequence of records:
> 
> rmgr: Storage len (rec/tot): 16/42, tx:  0, lsn: 
> 0/03015AF0, prev 0/03015958, desc: CREATE base/12669/16387
> rmgr: Heaplen (rec/tot):  3/  1518, tx:867, lsn: 
> 0/03015B20, prev 0/03015AF0, desc: INSERT off 8, blkref #0: rel 
> 1663/12669/1247 blk 8 FPW
> <...>
> rmgr: Btree   len (rec/tot):  2/72, tx:867, lsn: 
> 0/03019CD0, prev 0/03019C88, desc: INSERT_LEAF off 114, blkref #0: rel 
> 1663/12669/2674 blk 22
> rmgr: Standby len (rec/tot): 16/42, tx:867, lsn: 
> 0/03019D18, prev 0/03019CD0, desc: LOCK xid 867 db 12669 rel 16387 
> rmgr: Transaction len (rec/tot):784/   813, tx:867, lsn: 
> 0/03019D48, prev 0/03019D18, desc: PREPARE 
> rmgr: Transaction len (rec/tot):380/   409, tx:  0, lsn: 
> 0/0301A090, prev 0/03019D48, desc: COMMIT_PREPARED 867: 2016-04-08 
> 14:38:33.347851 MSK;
> 
> It looks like that you had stuck in LOCK xid 867 even before replaying 
> PREPARE record, so I have not that much ideas on why that can be caused by 
> changing procedures of PREPARE replay.
> 
> Just to keep things sane, here is my current diff:
> 
> 

Michael, it looks like that you are the only one person who can reproduce that 
bug. I’ve tried on bunch of OS’s and didn’t observe that behaviour, also 
looking at your backtraces I can’t get who is holding this lock (and all of 
that happens before first prepare record is replayed).

Can you investigate it more? Particularly find out who holds the lock?

There is last version of the patch:



twophase_replay.v4.patch
Description: Binary data

-- 
Stas Kelvich
Postgres Professional: http://www.postgrespro.com
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] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Thu, Apr 7, 2016 at 5:15 AM, Noah Misch  wrote:

> On Wed, Apr 06, 2016 at 09:17:22AM +0200, Magnus Hagander wrote:
> > On Wed, Apr 6, 2016 at 6:42 AM, Noah Misch  wrote:
> > > On Tue, Apr 05, 2016 at 08:15:16PM +0200, Magnus Hagander wrote:
> > > > I've pushed this version, and also added the item from the Brussels
> > > > developer meeting to actually rewrite the main backup docs to the
> open
> > > > items so they are definitely not forgotten for 9.6.
> > >
> > > Here's that PostgreSQL 9.6 open item:
> > >
> > > * Update of backup documentation (assigned to Bruce at [
> > > https://wiki.postgresql.org/wiki/FOSDEM/PGDay_2016_Developer_Meeting
> > > Brussels Developer Meeting], but others are surely allowed to work on
> it as
> > > well)
> > > ** Made required by 7117685461af50f50c03f43e6a622284c8d54694 since the
> > > current documentation is now incorrect
> > >
> > > Unless Bruce endorsed this development strategy, I think it unfair for
> > > commit
> > > 7117685 to impose a deadline on his backup.sgml project.  Did commit
> > > 7117685
> > >
> >
> > I agree that's a horrible wording. What it meant to imply was a deadline
> > for *me* to help take that off Bruce's shoulders before then while also
> > opening the doors for others to work on it should they want. Re-reading
> it
> > now I can see that's not at all what it says. I'll reword (or rather,
> > remove most of that, since it's been discussed separately and doesn't
> > actually need to go on the open items list which is a list and not a
> > discussion)
>
> Ahh.  Your original wording did admit your interpretation; I just didn't
> think
> of that interpretation.
>
> > > The chapter already does describe pg_basebackup before describing
> > > pg_start_backup; what else did the plan entail?
>
> > Recommending people to primarily use tried and tested ways of doing
> backups
> > (including a reference to a list, probably on the wiki, of known external
> > tools that "do things the right way", such as backrest or barman) rather
> > than focusing on examples that aren't necessarily even correct, and
> > certainly not complete.
> >
> > Mentioning the actual problems of exclusive base backups. (And obviously
> > talk about how using pg_start_backup/pg_stop_backup now makes it possible
> > to do "low level" backups without the risks of exclusive backups -- which
> > is the "incomplete" part from my note.
> >
> > More clearly outlining backup vs dump, and possibly (I can't actually
> > remember if we decided the second step) put base backups before pg_dump
> > since the topic is backups.
>
> Unless you especially want to self-impose the same tight resolution
> schedule
> that 9.6 regressions experience, let's move this to the "Non-bugs" section.
> Which do you prefer?  I don't think the opportunity for more documentation
> in
> light of 7117685 constitutes a regression, and I don't want "Open Issues"
> to
> double as a parking lot for slow-moving non-regressions.
>

Well, if we *don't* do the rewrite before we release it, then we have to
instead put information about the new version of the functions into the old
structure I think.

So I think it's an open issue. But maybe we should have a separate section
on the open items list for documentation issues? I tihnk we've had that
some times before.

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

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

[Masahiko Sawada]

On Mon, Apr 11, 2016 at 1:31 PM, Fujii Masao  wrote:
> On Mon, Apr 11, 2016 at 10:58 AM, Masahiko Sawada  
> wrote:
>> On Sat, Apr 9, 2016 at 12:32 PM, Tom Lane  wrote:
>>> Jeff Janes  writes:
 When I compile now without cassert, I get the compiler warning:
>>>
 syncrep.c: In function 'SyncRepUpdateConfig':
 syncrep.c:878:6: warning: variable 'parse_rc' set but not used
 [-Wunused-but-set-variable]
>>>
>>> If there's a good reason for that to be an Assert, I don't see it.
>>> There are no callers of SyncRepUpdateConfig that look like they
>>> need to, or should expect not to have to, tolerate errors.
>>> I think the way to fix this is to turn the Assert into a plain
>>> old test-and-ereport-ERROR.
>>>
>>
>> I've changed the draft patch Amit implemented so that it doesn't parse
>> twice(check_hook and assign_hook).
>> So assertion that was in assign_hook is no longer necessary.
>>
>> Please find attached.
>
> Thanks for the patch!
>
> When I emptied s_s_names, reloaded the configration file, set it to 'standby1'
> and reloaded the configuration file again, the master crashed with
> the following error.
>
> *** glibc detected *** postgres: wal sender process postgres [local]
> streaming 0/3015F18: munmap_chunk(): invalid pointer:
> 0x024d9a40 ***
> === Backtrace: =
> *** glibc detected *** postgres: wal sender process postgres [local]
> streaming 0/3015F18: munmap_chunk(): invalid pointer:
> 0x024d9a40 ***
> /lib64/libc.so.6[0x3be8e75f4e]
> postgres: wal sender process postgres [local] streaming 0/3015F18[0x97dae2]
> === Backtrace: =
> /lib64/libc.so.6[0x3be8e75f4e]
> postgres: wal sender process postgres [local] streaming
> 0/3015F18(set_config_option+0x12cb)[0x982242]
> postgres: wal sender process postgres [local] streaming
> 0/3015F18(SetConfigOption+0x4b)[0x9827ff]
> postgres: wal sender process postgres [local] streaming 0/3015F18[0x97dae2]
> postgres: wal sender process postgres [local] streaming 0/3015F18[0x988b4e]
> postgres: wal sender process postgres [local] streaming
> 0/3015F18(set_config_option+0x12cb)[0x982242]
> postgres: wal sender process postgres [local] streaming 0/3015F18[0x98af40]
> postgres: wal sender process postgres [local] streaming
> 0/3015F18(SetConfigOption+0x4b)[0x9827ff]
> postgres: wal sender process postgres [local] streaming
> 0/3015F18(ProcessConfigFile+0x9f)[0x98a98b]
> postgres: wal sender process postgres [local] streaming 0/3015F18[0x988b4e]
> postgres: wal sender process postgres [local] streaming 0/3015F18[0x98af40]
> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7b50fd]
> postgres: wal sender process postgres [local] streaming
> 0/3015F18(ProcessConfigFile+0x9f)[0x98a98b]
> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7b359c]
> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7b50fd]
> postgres: wal sender process postgres [local] streaming
> 0/3015F18(exec_replication_command+0x1a7)[0x7b47b6]
> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7b359c]
> postgres: wal sender process postgres [local] streaming
> 0/3015F18(PostgresMain+0x772)[0x8141b6]
> postgres: wal sender process postgres [local] streaming
> 0/3015F18(exec_replication_command+0x1a7)[0x7b47b6]
> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7896f7]
> postgres: wal sender process postgres [local] streaming
> 0/3015F18(PostgresMain+0x772)[0x8141b6]
> postgres: wal sender process postgres [local] streaming 0/3015F18[0x788e62]
> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7896f7]
> postgres: wal sender process postgres [local] streaming 0/3015F18[0x785544]
> postgres: wal sender process postgres [local] streaming 0/3015F18[0x788e62]
> postgres: wal sender process postgres [local] streaming
> 0/3015F18(PostmasterMain+0x1134)[0x784c08]
> postgres: wal sender process postgres [local] streaming 0/3015F18[0x785544]
> postgres: wal sender process postgres [local] streaming 0/3015F18[0x6ce12e]
> /lib64/libc.so.6(__libc_start_main+0xfd)[0x3be8e1ed5d]
> postgres: wal sender process postgres [local] streaming
> 0/3015F18(PostmasterMain+0x1134)[0x784c08]
> postgres: wal sender process postgres [local] streaming 0/3015F18[0x467e99]
>

Thank you for reviewing.

SyncRepUpdateConfig() seems to be no longer necessary.
Attached updated version.

Regards,

--
Masahiko Sawada
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index cb6b5e5..93dd836 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -68,6 +68,7 @@
 #include "storage/proc.h"
 #include "tcop/tcopprot.h"
 #include "utils/builtins.h"
+#include "utils/memutils.h"
 #include "utils/ps_status.h"
 
 /* User-settable parameters for sync rep */
@@ -361,11 +362,6 @@ SyncRepInitConfig(void)
 {
 	int			priority;
 
-	/* Update the 

Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW

[Etsuro Fujita]

On 2016/04/11 12:30, Michael Paquier wrote:

+   if ((cancel = PQgetCancel(entry->conn)))
+   {
+   PQcancel(cancel, errbuf, sizeof(errbuf));
+   PQfreeCancel(cancel);
+   }
Wouldn't it be better to issue a WARNING here if PQcancel does not succeed?


Seems like a good idea.  Attached is an updated version of the patch.

Thanks for the review!

Best regards,
Etsuro Fujita
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 189f290..da1758b 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -598,6 +598,34 @@ pgfdw_xact_callback(XactEvent event, void *arg)
 case XACT_EVENT_ABORT:
 	/* Assume we might have lost track of prepared statements */
 	entry->have_error = true;
+	/*
+	 * If we had submitted a command to the remote server using
+	 * an asynchronous execution function, the command might
+	 * have not yet completed.  Check to see if a command is
+	 * still being processed by the remote server, and if so,
+	 * request cancellation of the command; if not, abort
+	 * gracefully.
+	 */
+	if (PQtransactionStatus(entry->conn) == PQTRANS_ACTIVE)
+	{
+		PGcancel   *cancel;
+		char		errbuf[256];
+
+		if ((cancel = PQgetCancel(entry->conn)))
+		{
+			/*
+			 * Note: can't throw ERROR, it would be infinite
+			 * loop
+			 */
+			if (!PQcancel(cancel, errbuf, sizeof(errbuf)))
+ereport(WARNING,
+		(errcode(ERRCODE_CONNECTION_FAILURE),
+		 errmsg("could not send cancel request: %s",
+errbuf)));
+			PQfreeCancel(cancel);
+		}
+		break;
+	}
 	/* If we're aborting, abort all remote transactions too */
 	res = PQexec(entry->conn, "ABORT TRANSACTION");
 	/* Note: can't throw ERROR, it would be infinite loop */
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index ee0220a..0378f1d 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -32,6 +32,7 @@
 #include "optimizer/var.h"
 #include "optimizer/tlist.h"
 #include "parser/parsetree.h"
+#include "storage/latch.h"
 #include "utils/builtins.h"
 #include "utils/guc.h"
 #include "utils/lsyscache.h"
@@ -3131,6 +3132,7 @@ execute_dml_stmt(ForeignScanState *node)
 	ExprContext *econtext = node->ss.ps.ps_ExprContext;
 	int			numParams = dmstate->numParams;
 	const char **values = dmstate->param_values;
+	int			wc;
 
 	/*
 	 * Construct array of query parameter values in text format.
@@ -3147,12 +3149,42 @@ execute_dml_stmt(ForeignScanState *node)
 	 * parameter (see deparse.c), the "inference" is trivial and will produce
 	 * the desired result.  This allows us to avoid assuming that the remote
 	 * server has the same OIDs we do for the parameters' types.
+	 */
+	if (!PQsendQueryParams(dmstate->conn, dmstate->query, numParams,
+		   NULL, values, NULL, NULL, 0))
+		pgfdw_report_error(ERROR, NULL, dmstate->conn, false, dmstate->query);
+
+	/*
+	 * Receive data until PQgetResult is ready to get the result without
+	 * blocking.
+	 */
+	while (PQisBusy(dmstate->conn))
+	{
+		/* Sleep until there's something to do */
+		wc = WaitLatchOrSocket(MyLatch,
+			   WL_LATCH_SET | WL_SOCKET_READABLE,
+			   PQsocket(dmstate->conn),
+			   -1L);
+		ResetLatch(MyLatch);
+
+		CHECK_FOR_INTERRUPTS();
+
+		/* Data available in socket */
+		if (wc & WL_SOCKET_READABLE)
+		{
+			if (!PQconsumeInput(dmstate->conn))
+pgfdw_report_error(ERROR, NULL, dmstate->conn, false,
+   dmstate->query);
+		}
+	}
+
+	/*
+	 * Get the result
 	 *
 	 * We don't use a PG_TRY block here, so be careful not to throw error
 	 * without releasing the PGresult.
 	 */
-	dmstate->result = PQexecParams(dmstate->conn, dmstate->query,
-   numParams, NULL, values, NULL, NULL, 0);
+	dmstate->result = PQgetResult(dmstate->conn);
 	if (PQresultStatus(dmstate->result) !=
 		(dmstate->has_returning ? PGRES_TUPLES_OK : PGRES_COMMAND_OK))
 		pgfdw_report_error(ERROR, dmstate->result, dmstate->conn, 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] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
 wrote:
> Now, I have produced two patches:
> - 0001-Support-for-VS-2015-locale-hack.patch, which makes use of
> __crt_locale_data_public in ucrt/corecrt.h. This is definitely an ugly
> hack, though I am coming to think that this may be the approach that
> would us the less harm, and that's closer to what is done for VS 2012
> and 2013.
> - 0001-Support-for-VS-2015-getlocaleinfoex.patch, which make use of
> GetLocaleInfoEx, this requires us to lower a bit the support grid for
> Windows, basically that's throwing support for XP if compilation is
> done with VS 2015.
> Based on my tests, both are working with short and long local names,
> testing via initdb --locale.

The first patch is actually not what I wanted to send. Here are the
correct ones...
-- 
Michael


0001-Support-for-VS-2015-getlocaleinfoex.patch
Description: binary/octet-stream


0001-Support-for-VS-2015-locale-hack.patch
Description: binary/octet-stream

-- 
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] Some other things about contrib/bloom and generic_xlog.c

[Michael Paquier]

On Sun, Apr 10, 2016 at 1:49 PM, Tom Lane  wrote:
> 1. It doesn't seem like generic_xlog.c has thought very carefully about
> the semantics of the "hole" between pd_lower and pd_upper.  The mainline
> XLOG code goes to some lengths to ensure that the hole stays all-zeroes;
> for example RestoreBlockImage() explicitly zeroes the hole when restoring
> from a full-page image that has a hole.

Yes.

> But generic_xlog.c's redo routine
> does not do anything comparable, nor does GenericXLogFinish make any
> effort to ensure that the "hole" is all-zeroes after normal application of
> a generic update.  The reason this is of interest is that it means the
> contents of the "hole" could diverge between master and slave, or differ
> between the original state of a database and what it is after a crash and
> recovery.  That would at least complicate forensic comparisons of pages,
> and I think it might also break checksumming.  We thought that this was
> important enough to take the trouble of explicitly zeroing holes during
> mainline XLOG replay.  Shouldn't generic_xlog.c take the same trouble?

One look at checksum_impl.h shows up that the page hole is taken into
account in the checksum calculation, so we definitely has better zero
the page. And looking at generic_xlog.c, this code either logs in a
full page, or a delta.

Actually, as I look at this code for the first time, I find that
GenericXLogFinish() is a very confusing interface. It makes no
distinction between a page and the meta data associated to this page,
this is controlled by a flag isNew and buffer data is saved as some
delta. Actually, fullmage is not exactly true, this may refer to a
page that has a hole. It seems to me that we should not have one but
two routines: GenericXLogRegisterBuffer and
GenericXLogRegisterBufData, similar to what the normal XLOG routines
offer.
-- 
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] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Sat, Apr 9, 2016 at 12:32 PM, Tom Lane  wrote:
> Jeff Janes  writes:
>> When I compile now without cassert, I get the compiler warning:
>
>> syncrep.c: In function 'SyncRepUpdateConfig':
>> syncrep.c:878:6: warning: variable 'parse_rc' set but not used
>> [-Wunused-but-set-variable]

Thanks for the report!

> If there's a good reason for that to be an Assert, I don't see it.
> There are no callers of SyncRepUpdateConfig that look like they
> need to, or should expect not to have to, tolerate errors.
> I think the way to fix this is to turn the Assert into a plain
> old test-and-ereport-ERROR.

Okay, I pushed that change. Thanks for the suggestion!

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] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Mon, Apr 11, 2016 at 6:03 AM, Petr Jelinek  wrote:
> On 10/04/16 20:47, Christian Ullrich wrote:
>>> don't think we need to be too precious about saving a byte or two
>>> here. Can one or other of you please prepare a replacement patch based
>>> in this discussion?

So, I am finally back on the battlefield.

>> Sorry, I don't think I'm up to that (at least not for another week or
>> so). I have to read up on the issue first; right now I'm not sure what
>> exactly the problem is.
>>
>> What I can say is that the existing patch needs more work, because
>> GetLocaleInfoEx() expects a locale name ("de-DE") as its first argument,
>> but the patched win32_langinfo() is giving it a locale identifier
>> ("German_Germany.1252"). At least it does for me.
>
>
> That really depends on what you set in config/commandline. The current code
> supports both (that's why there is the else fallback to old code which
> handles the "German_Germany.1252" format).

Yes, that's the whole point of falling back to the old code path
should the call to GetLocaleInfoEx() fail.

>> As for missing code page information in the _locale_t type, ISTM it
>> isn't hidden any better in UCRT than it was before:
>>
>> int main()
>> {
>>  /* Set locale with nonstandard code page */
>>  _locale_t loc = _create_locale(LC_ALL, "German_Germany.850");
>>
>>  __crt_locale_data_public* pub =
>> (__crt_locale_data_public*)(loc->locinfo);
>>  printf("CP: %d\n", pub->_locale_lc_codepage);// "CP: 850"
>>  return 0;
>> }
>>
>
> This is basically same as the approach of fixing _locale_t that was also
> considered upthread but nobody was too happy with it.

I am one of them to be honest... Now if I look at that with one step
backward at this problem this requires just a ugly hack of a couple of
lines, and this does not require to bump __WIN32_WINNT... Neither does
it need an extra code hunk to handle the short locale name parsing
with GetLocaleInfoEx. We could even just handle _MSC_VER as an
exception, so as it is easy to check with future versions of VS if we
still have lc_codepage going missing:
+#if (_MSC_VER == 1900)
+   __crt_locale_data_public *pub =
+   (__crt_locale_data_public *) loct->locinfo;
+   lc_codepage = pub->_locale_lc_codepage;
+#else
+   lc_codepage = loct->locinfo->lc_codepage;
+#endif

Another thing that I am wondering is that this would allow us to parse
even locale names that are not short, type ja-JP and not with the
COUNTRY_LANG.CODEPAGE format, though based on what I read on the docs
those do not exist.. But the world of Windows is full of surprises.

> I guess the worry is
> that given that the header file is obviously unfinished in the area where
> this is defined and also the fact that this looks like something that's not
> meant to be used outside of that header makes people worry that it might
> change between point releases of the SDK.

Yep.

Now, I have produced two patches:
- 0001-Support-for-VS-2015-locale-hack.patch, which makes use of
__crt_locale_data_public in ucrt/corecrt.h. This is definitely an ugly
hack, though I am coming to think that this may be the approach that
would us the less harm, and that's closer to what is done for VS 2012
and 2013.
- 0001-Support-for-VS-2015-getlocaleinfoex.patch, which make use of
GetLocaleInfoEx, this requires us to lower a bit the support grid for
Windows, basically that's throwing support for XP if compilation is
done with VS 2015.
Based on my tests, both are working with short and long local names,
testing via initdb --locale.
-- 
Michael


0001-Support-for-VS-2015-getlocaleinfoex.patch
Description: binary/octet-stream


0001-Support-for-VS-2015-locale-hack.patch
Description: binary/octet-stream

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