Re: [COMMITTERS] pgsql: Generic Messages for Logical Decoding

2016-04-06 Thread Andres Freund 
On 2016-04-07 12:26:28 +0900, Fujii Masao wrote:
> In my example, the WAL record of INSERT that I executed last should be in
> 00010005. But pg_xlogdump could not display that.
> The output of pg_xlogdump was:
> 
> $ pg_xlogdump data/pg_xlog/00010005
> pg_xlogdump: FATAL:  could not find a valid record after 0/500
> 
> ISTM that if a WAL file starts with the latter half of LOGICAL MESSAGE
> WAL data, pg_xlogdump treats it as invalid and gives up dumping the
> remaining WAL data in the file.

That'd obviously be something to investigate. IIRC there's a thread
nearby about something like this. But just to confirm, if you use -s
over multiple records it works?

Greetings,

Andres Freund


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

[COMMITTERS] pgsql: Remove redundant message in AddUserToTokenDacl().

2016-04-06 Thread Noah Misch 
Remove redundant message in AddUserToTokenDacl().

GetTokenUser() will have reported an adequate error message.  These
error conditions almost can't happen, so users are unlikely to observe
this change.

Reviewed by Tom Lane and Stephen Frost.

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/33d3fc5e2aac32fcf356c09cee4bfded6613a1f3

Modified Files
--
src/common/exec.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)


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

[COMMITTERS] pgsql: Standardize GetTokenInformation() error reporting.

2016-04-06 Thread Noah Misch 
Standardize GetTokenInformation() error reporting.

Commit c22650cd6450854e1a75064b698d7dcbb4a8821a sparked a discussion
about diverse interpretations of "token user" in error messages.  Expel
old and new specimens of that phrase by making all GetTokenInformation()
callers report errors the way GetTokenUser() has been reporting them.
These error conditions almost can't happen, so users are unlikely to
observe this change.

Reviewed by Tom Lane and Stephen Frost.

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/f2b1b3079ce9d2965f6e450585f24d18cdf5647b

Modified Files
--
src/backend/libpq/auth.c  | 8 
src/port/win32security.c  | 4 ++--
src/test/regress/pg_regress.c | 4 ++--
3 files changed, 8 insertions(+), 8 deletions(-)


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

Re: [COMMITTERS] pgsql: Generic Messages for Logical Decoding

2016-04-06 Thread Fujii Masao 
On Thu, Apr 7, 2016 at 12:06 AM, Andres Freund  wrote:
>
>
> On April 6, 2016 5:00:54 PM GMT+02:00, Fujii Masao  
> wrote:
>>On Wed, Apr 6, 2016 at 6:08 PM, Simon Riggs 
>>wrote:
>>> Generic Messages for Logical Decoding
>>>
>>> API and mechanism to allow generic messages to be inserted into WAL
>>that are
>>> intended to be read by logical decoding plugins. This commit adds an
>>optional
>>> new callback to the logical decoding API.
>>
>>When I specify very long text message, pg_xlogdump failed to dump
>>correctly the WAL file which should contain that inserted text message.
>>Isn't this a bug?
>>
>>You can reproduce the problem by the following steps.
>>
>>=# SELECT pg_xlogfile_name(pg_switch_xlog());
>> pg_xlogfile_name
>>--
>> 00010003
>>(1 row)
>>
>>=# SELECT pg_xlogfile_name(pg_current_xlog_location());
>> pg_xlogfile_name
>>--
>> 00010003
>>(1 row)
>>
>>=# SELECT pg_logical_emit_message(true, 'test',
>>repeat('0123456789ABCDEFG', 1024*1024));
>> pg_logical_emit_message
>>-
>> 0/510CD40
>>(1 row)
>>
>>=# SELECT pg_xlogfile_name(pg_current_xlog_location());
>> pg_xlogfile_name
>>--
>> 00010005
>>(1 row)
>>
>>=# insert into t values(0,0);
>>INSERT 0 1
>>postgres=# SELECT pg_xlogfile_name(pg_current_xlog_location());
>> pg_xlogfile_name
>>--
>> 00010005
>>(1 row)
>>
>>The WAL record of pg_logical_emit_message() should be stored in
>>00010004 and 00010005.
>>The WAL record of last insertion should be stored in
>>00010005.
>>But the results of pg_xlogdump were wrong as follows.
>>
>>$ pg_xlogdump data/pg_xlog/00010004
>>rmgr: Standby len (rec/tot): 24/50, tx:  0, lsn:
>>0/0428, prev 0/030146F8, desc: RUNNING_XACTS nextXid 863
>>latestCompletedXid 862 oldestRunningXid 863
>>
>>$ pg_xlogdump data/pg_xlog/00010005
>>pg_xlogdump: FATAL:  could not find a valid record after 0/500
>
> If you specify a file it only looks at records in that file. Try with -s.

In my example, the WAL record of INSERT that I executed last should be in
00010005. But pg_xlogdump could not display that.
The output of pg_xlogdump was:

$ pg_xlogdump data/pg_xlog/00010005
pg_xlogdump: FATAL:  could not find a valid record after 0/500

ISTM that if a WAL file starts with the latter half of LOGICAL MESSAGE
WAL data, pg_xlogdump treats it as invalid and gives up dumping the
remaining WAL data in the file.

Regards,

-- 
Fujii Masao


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

Re: [COMMITTERS] pgsql: Use GRANT system to manage access to sensitive functions

2016-04-06 Thread Fujii Masao 
On Thu, Apr 7, 2016 at 10:45 AM, Stephen Frost  wrote:
> Use GRANT system to manage access to sensitive functions
>
> Now that pg_dump will properly dump out any ACL changes made to
> functions which exist in pg_catalog, switch to using the GRANT system
> to manage access to those functions.
>
> This means removing 'if (!superuser()) ereport()' checks from the
> functions themselves and then REVOKEing EXECUTE right from 'public' for
> these functions in system_views.sql.

This commit revokes the execution privilege on pg_start_backup() from
a replication role. Doesn't this affect many systems that a replication
role is used to take a backup? This commit forces administrators of
those systems to manually grant the privilege to a replication role
when upgrading the system to 9.6.

Regards,

-- 
Fujii Masao


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

[COMMITTERS] pgsql: Bump catversion for pg_dump dump catalog ACL patches

2016-04-06 Thread Stephen Frost 
Bump catversion for pg_dump dump catalog ACL patches

Pointed out by Tom.

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/29dd1504a12f324c75f6b5ce8863505e499633ec

Modified Files
--
src/include/catalog/catversion.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)


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

Re: [COMMITTERS] pgsql: Use GRANT system to manage access to sensitive functions

2016-04-06 Thread Stephen Frost 
* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > Use GRANT system to manage access to sensitive functions
> 
> This patch series seems approximately three catversion bumps
> shy of a load ...

Blargh.  I told myself at three different times tonight to remember to
bump catversion.

Apologies, will do so momentairly.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [COMMITTERS] pgsql: Use GRANT system to manage access to sensitive functions

2016-04-06 Thread Tom Lane 
Stephen Frost  writes:
> Use GRANT system to manage access to sensitive functions

This patch series seems approximately three catversion bumps
shy of a load ...

regards, tom lane


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

Re: [COMMITTERS] pgsql: Use GRANT system to manage access to sensitive functions

2016-04-06 Thread Michael Paquier 
On Thu, Apr 7, 2016 at 10:45 AM, Stephen Frost  wrote:
> Use GRANT system to manage access to sensitive functions
>
> Now that pg_dump will properly dump out any ACL changes made to
> functions which exist in pg_catalog, switch to using the GRANT system
> to manage access to those functions.
>
> This means removing 'if (!superuser()) ereport()' checks from the
> functions themselves and then REVOKEing EXECUTE right from 'public' for
> these functions in system_views.sql.

+1.
-- 
Michael


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

[COMMITTERS] pgsql: In pg_dump, split "dump" into "dump" and "dump_contains"

2016-04-06 Thread Stephen Frost 
In pg_dump, split "dump" into "dump" and "dump_contains"

Historically, the "dump" component of the namespace has been used
to decide if the objects inside of the namespace should be dumped
also.  Given that "dump" is now a bitmask and may be partial, and
we may want to dump out all components of the namespace object but
only some of the components of objects contained in the namespace,
create a "dump_contains" bitmask which will represent what components
of the objects inside of a namespace should be dumped out.

No behavior change here, but in preparation for a change where we
will dump out just the ACLs of objects in pg_catalog, but we might
not dump out the ACL of the pg_catalog namespace itself (for instance,
when it hasn't been changed from the value set at initdb time).

Reviews by Alexander Korotkov, Jose Luis Tallon

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/d217b2c360cb9a746b4ef122c568bdfedb6d726e

Modified Files
--
src/bin/pg_dump/pg_dump.c | 23 +--
src/bin/pg_dump/pg_dump.h |  3 ++-
2 files changed, 15 insertions(+), 11 deletions(-)


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

[COMMITTERS] pgsql: Add new catalog called pg_init_privs

2016-04-06 Thread Stephen Frost 
Add new catalog called pg_init_privs

This new catalog holds the privileges which the system was
initialized with at initdb time, along with any permissions set
by extensions at CREATE EXTENSION time.  This allows pg_dump
(and any other similar use-cases) to detect when the privileges
set on initdb-created or extension-created objects have been
changed from what they were set to at initdb/extension-creation
time and handle those changes appropriately.

Reviews by Alexander Korotkov, Jose Luis Tallon

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/6c268df1276e9dd73e4d2cc89cf8787e8f186bda

Modified Files
--
doc/src/sgml/catalogs.sgml | 108 +
src/backend/catalog/Makefile   |   2 +-
src/backend/catalog/aclchk.c   | 149 +
src/backend/catalog/dependency.c   |  46 -
src/bin/initdb/initdb.c| 143 +++
src/include/catalog/indexing.h |   3 +
src/include/catalog/pg_init_privs.h| 101 +++
src/test/regress/expected/sanity_check.out |   1 +
8 files changed, 549 insertions(+), 4 deletions(-)


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

[COMMITTERS] pgsql: In pg_dump, use a bitmap to represent what to include

2016-04-06 Thread Stephen Frost 
In pg_dump, use a bitmap to represent what to include

pg_dump has historically used a simple boolean 'dump' value to indicate
if a given object should be included in the dump or not.  Instead, use
a bitmap which breaks down the components of an object into their
distinct pieces and use that bitmap to only include the components
requested.

This does not include any behavioral change, but is in preperation for
the change to dump out just ACLs for objects in pg_catalog.

Reviews by Alexander Korotkov, Jose Luis Tallon

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/a9f0e8e5a2e779a888988cb64479a6723f668c84

Modified Files
--
src/bin/pg_dump/common.c  |2 +-
src/bin/pg_dump/pg_dump.c | 1502 -
src/bin/pg_dump/pg_dump.h |   14 +-
3 files changed, 833 insertions(+), 685 deletions(-)


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

[COMMITTERS] pgsql: In pg_dump, include pg_catalog and extension ACLs, if changed

2016-04-06 Thread Stephen Frost 
In pg_dump, include pg_catalog and extension ACLs, if changed

Now that all of the infrastructure exists, add in the ability to
dump out the ACLs of the objects inside of pg_catalog or the ACLs
for objects which are members of extensions, but only if they have
been changed from their original values.

The original values are tracked in pg_init_privs.  When pg_dump'ing
9.6-and-above databases, we will dump out the ACLs for all objects
in pg_catalog and the ACLs for all extension members, where the ACL
has been changed from the original value which was set during either
initdb or CREATE EXTENSION.

This should not change dumps against pre-9.6 databases.

Reviews by Alexander Korotkov, Jose Luis Tallon

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/23f34fa4ba358671adab16773e79c17c92cbc870

Modified Files
--
doc/src/sgml/extend.sgml   |   21 +
src/backend/catalog/aclchk.c   |   17 +-
src/backend/utils/adt/pg_upgrade_support.c |   12 +
src/bin/initdb/initdb.c|6 +-
src/bin/pg_dump/dumputils.c|  274 ++-
src/bin/pg_dump/dumputils.h|9 +-
src/bin/pg_dump/pg_dump.c  | 1077 +++-
src/bin/pg_dump/pg_dump.h  |   24 +
src/bin/pg_dump/pg_dumpall.c   |6 +-
src/include/catalog/binary_upgrade.h   |2 +
src/include/catalog/pg_proc.h  |2 +
src/test/regress/expected/init_privs.out   |   13 +
src/test/regress/parallel_schedule |2 +-
src/test/regress/serial_schedule   |1 +
src/test/regress/sql/init_privs.sql|   11 +
15 files changed, 1268 insertions(+), 209 deletions(-)


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

[COMMITTERS] pgsql: Use GRANT system to manage access to sensitive functions

2016-04-06 Thread Stephen Frost 
Use GRANT system to manage access to sensitive functions

Now that pg_dump will properly dump out any ACL changes made to
functions which exist in pg_catalog, switch to using the GRANT system
to manage access to those functions.

This means removing 'if (!superuser()) ereport()' checks from the
functions themselves and then REVOKEing EXECUTE right from 'public' for
these functions in system_views.sql.

Reviews by Alexander Korotkov, Jose Luis Tallon

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/1574783b4ced0356fbc626af1a1a469faa6b41e1

Modified Files
--
doc/src/sgml/backup.sgml   |  8 +++--
doc/src/sgml/func.sgml | 19 ++--
doc/src/sgml/monitoring.sgml   | 12 +---
src/backend/access/transam/xlogfuncs.c | 56 +-
src/backend/catalog/system_views.sql   | 21 +
src/backend/postmaster/pgstat.c| 24 ++-
src/backend/utils/adt/misc.c   | 16 --
7 files changed, 81 insertions(+), 75 deletions(-)


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

Re: [COMMITTERS] pgsql: Implement backup API functions for non-exclusive backups

2016-04-06 Thread Magnus Hagander 
On Wed, Apr 6, 2016 at 6:38 PM, Tom Lane  wrote:

> Magnus Hagander  writes:
> > Implement backup API functions for non-exclusive backups
>
> Shouldn't the CF entry for this be closed?  The docs rewrite
> seems like a separate task.
>
>
It should. I forgot, thanks for the reminder. Done now.


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

Re: [COMMITTERS] pgsql: Implement backup API functions for non-exclusive backups

2016-04-06 Thread Tom Lane 
Magnus Hagander  writes:
> Implement backup API functions for non-exclusive backups

Shouldn't the CF entry for this be closed?  The docs rewrite
seems like a separate task.

regards, tom lane


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

[COMMITTERS] pgsql: Add jsonb_insert

2016-04-06 Thread Teodor Sigaev 
Add jsonb_insert

It inserts a new value into an jsonb array at arbitrary position or
a new key to jsonb object.

Author: Dmitry Dolgov
Reviewers: Petr Jelinek, Vitaly Burovoy, Andrew Dunstan

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/0b62fd036e1ac48a8432bb9664b21e1f036c1b08

Modified Files
--
doc/src/sgml/func.sgml   |  45 ++--
src/backend/catalog/system_views.sql |   8 +++
src/backend/utils/adt/jsonfuncs.c| 134 ---
src/include/catalog/catversion.h |   2 +-
src/include/catalog/pg_proc.h|   2 +
src/include/utils/jsonb.h|   3 +
src/test/regress/expected/jsonb.out  | 129 +
src/test/regress/sql/jsonb.sql   |  30 
8 files changed, 324 insertions(+), 29 deletions(-)


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

[COMMITTERS] pgsql: pg_dump: Add table qualifications to some tags

2016-04-06 Thread Peter Eisentraut 
pg_dump: Add table qualifications to some tags

Some object types have names that are only unique for one table.  But
for those we generally didn't put the table name into the dump TOC tag.
So it was impossible to identify these objects if the same name was used
for multiple tables.  This affects policies, column defaults,
constraints, triggers, and rules.

Fix by adding the table name to the TOC tag, so that it now reads
"$schema $table $object".

Reviewed-by: Michael Paquier 

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/3b3fcc4eeaeecff315420833975e7c87d760bfe1

Modified Files
--
src/bin/pg_dump/pg_dump.c | 42 ++
1 file changed, 34 insertions(+), 8 deletions(-)


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

[COMMITTERS] pgsql: Run pgindent on a batch of (mostly-planner-related) source files

2016-04-06 Thread Tom Lane 
Run pgindent on a batch of (mostly-planner-related) source files.

Getting annoyed at the amount of unrelated chatter I get from pgindent'ing
Rowley's unique-joins patch.  Re-indent all the files it touches.

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/de94e2af184e25576b13cbda8cf825118835d1cd

Modified Files
--
contrib/postgres_fdw/postgres_fdw.c   | 28 +--
src/backend/commands/explain.c| 17 ---
src/backend/nodes/copyfuncs.c |  2 +-
src/backend/nodes/equalfuncs.c|  2 +-
src/backend/nodes/outfuncs.c  |  2 +-
src/backend/optimizer/path/costsize.c |  6 +--
src/backend/optimizer/path/joinpath.c | 95 ++-
src/backend/optimizer/plan/planmain.c |  8 +--
src/backend/optimizer/plan/setrefs.c  | 34 ++---
src/backend/optimizer/util/relnode.c  | 14 +++---
src/backend/parser/parse_clause.c |  4 +-
src/include/nodes/nodes.h |  6 +--
src/include/nodes/relation.h  |  9 ++--
src/include/optimizer/planmain.h  |  5 +-
14 files changed, 118 insertions(+), 114 deletions(-)


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

Re: [COMMITTERS] pgsql: Generic Messages for Logical Decoding

2016-04-06 Thread Andres Freund 


On April 6, 2016 5:00:54 PM GMT+02:00, Fujii Masao  
wrote:
>On Wed, Apr 6, 2016 at 6:08 PM, Simon Riggs 
>wrote:
>> Generic Messages for Logical Decoding
>>
>> API and mechanism to allow generic messages to be inserted into WAL
>that are
>> intended to be read by logical decoding plugins. This commit adds an
>optional
>> new callback to the logical decoding API.
>
>When I specify very long text message, pg_xlogdump failed to dump
>correctly the WAL file which should contain that inserted text message.
>Isn't this a bug?
>
>You can reproduce the problem by the following steps.
>
>=# SELECT pg_xlogfile_name(pg_switch_xlog());
> pg_xlogfile_name
>--
> 00010003
>(1 row)
>
>=# SELECT pg_xlogfile_name(pg_current_xlog_location());
> pg_xlogfile_name
>--
> 00010003
>(1 row)
>
>=# SELECT pg_logical_emit_message(true, 'test',
>repeat('0123456789ABCDEFG', 1024*1024));
> pg_logical_emit_message
>-
> 0/510CD40
>(1 row)
>
>=# SELECT pg_xlogfile_name(pg_current_xlog_location());
> pg_xlogfile_name
>--
> 00010005
>(1 row)
>
>=# insert into t values(0,0);
>INSERT 0 1
>postgres=# SELECT pg_xlogfile_name(pg_current_xlog_location());
> pg_xlogfile_name
>--
> 00010005
>(1 row)
>
>The WAL record of pg_logical_emit_message() should be stored in
>00010004 and 00010005.
>The WAL record of last insertion should be stored in
>00010005.
>But the results of pg_xlogdump were wrong as follows.
>
>$ pg_xlogdump data/pg_xlog/00010004
>rmgr: Standby len (rec/tot): 24/50, tx:  0, lsn:
>0/0428, prev 0/030146F8, desc: RUNNING_XACTS nextXid 863
>latestCompletedXid 862 oldestRunningXid 863
>
>$ pg_xlogdump data/pg_xlog/00010005
>pg_xlogdump: FATAL:  could not find a valid record after 0/500

If you specify a file it only looks at records in that file. Try with -s.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.


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

Re: [COMMITTERS] pgsql: Generic Messages for Logical Decoding

2016-04-06 Thread Fujii Masao 
On Wed, Apr 6, 2016 at 6:08 PM, Simon Riggs  wrote:
> Generic Messages for Logical Decoding
>
> API and mechanism to allow generic messages to be inserted into WAL that are
> intended to be read by logical decoding plugins. This commit adds an optional
> new callback to the logical decoding API.

When I specify very long text message, pg_xlogdump failed to dump
correctly the WAL file which should contain that inserted text message.
Isn't this a bug?

You can reproduce the problem by the following steps.

=# SELECT pg_xlogfile_name(pg_switch_xlog());
 pg_xlogfile_name
--
 00010003
(1 row)

=# SELECT pg_xlogfile_name(pg_current_xlog_location());
 pg_xlogfile_name
--
 00010003
(1 row)

=# SELECT pg_logical_emit_message(true, 'test',
repeat('0123456789ABCDEFG', 1024*1024));
 pg_logical_emit_message
-
 0/510CD40
(1 row)

=# SELECT pg_xlogfile_name(pg_current_xlog_location());
 pg_xlogfile_name
--
 00010005
(1 row)

=# insert into t values(0,0);
INSERT 0 1
postgres=# SELECT pg_xlogfile_name(pg_current_xlog_location());
 pg_xlogfile_name
--
 00010005
(1 row)

The WAL record of pg_logical_emit_message() should be stored in
00010004 and 00010005.
The WAL record of last insertion should be stored in
00010005.
But the results of pg_xlogdump were wrong as follows.

$ pg_xlogdump data/pg_xlog/00010004
rmgr: Standby len (rec/tot): 24/50, tx:  0, lsn:
0/0428, prev 0/030146F8, desc: RUNNING_XACTS nextXid 863
latestCompletedXid 862 oldestRunningXid 863

$ pg_xlogdump data/pg_xlog/00010005
pg_xlogdump: FATAL:  could not find a valid record after 0/500

Regards,

-- 
Fujii Masao


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

[COMMITTERS] pgsql: Modify test_decoding/messages to remove non-ascii chars

2016-04-06 Thread Simon Riggs 
Modify test_decoding/messages to remove non-ascii chars

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/d25379eb23383f1d2f969e65e0332b47c19aea94

Modified Files
--
contrib/test_decoding/expected/messages.out | 14 +++---
contrib/test_decoding/sql/messages.sql  |  2 +-
2 files changed, 8 insertions(+), 8 deletions(-)


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

Re: [COMMITTERS] pgsql: Generic Messages for Logical Decoding

2016-04-06 Thread Simon Riggs 
On 6 April 2016 at 14:35, Tom Lane  wrote:

> Simon Riggs  writes:
> > Generic Messages for Logical Decoding
>
> The buildfarm thinks this was a remarkably poor choice of random data
> payload:
>
> SELECT 'žluťoučký kůň' FROM pg_logical_emit_message(true, 'test',
> 'žluťoučký kůň');
> ?column?
>  ---
>   žluťoučký kůň
>
> Since this test is not, so far as I can see, at all interested in
> character encoding questions, I suggest not using non-ASCII data in it.
>

Yes, working on it now.

This was my bad, since I requested it be added.

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

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

Re: [COMMITTERS] pgsql: Generic Messages for Logical Decoding

2016-04-06 Thread Tom Lane 
Simon Riggs  writes:
> Generic Messages for Logical Decoding

The buildfarm thinks this was a remarkably poor choice of random data
payload:

SELECT 'žluťoučký kůň' FROM pg_logical_emit_message(true, 'test', 
'žluťoučký kůň');
?column?
 ---
  žluťoučký kůň

Since this test is not, so far as I can see, at all interested in
character encoding questions, I suggest not using non-ASCII data in it.

regards, tom lane


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

Re: [HACKERS] [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

2016-04-06 Thread Andres Freund 
On 2016-04-06 14:00:20 +0100, Simon Riggs wrote:
> On 6 April 2016 at 13:48, Michael Paquier  wrote:
> 
> >
> > Yeah... We have reached a clear consensus about the way things should
> > be done after quite a lot of discussions that has gone for a couple of
> > months. And Andres' design on the matter is what is getting approval
> > from everybody who has worked toward addressing this issue while
> > really taking care of the problem at its root.

> Clearly I am not included in your use of the words "we" and "everybody" !

Uh. You're not serious, right? Obviously that refers to the months of
discussions that already had happened. Where you didn't participate,
hence obviously weren't included in "we".

Andres


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

[COMMITTERS] pgsql: Use proper format specifier %X/%X for LSN, again.

2016-04-06 Thread Fujii Masao 
Use proper format specifier %X/%X for LSN, again.

Commit cee31f5 fixed this problem, but commit 989be08 accidentally
reverted the fix.

Thomas Munro

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/ead9963c471ccde50ff220e8294ea11a57eee91c

Modified Files
--
src/backend/replication/syncrep.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)


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

Re: [HACKERS] [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

2016-04-06 Thread Andres Freund 
On 2016-04-06 13:50:24 +0100, Simon Riggs wrote:
> On 6 April 2016 at 13:27, Andres Freund  wrote:
> 
> > On 2016-04-06 13:11:40 +0100, Simon Riggs wrote:
> > > On 6 April 2016 at 10:09, Andres Freund  wrote:
> > > > On 2016-04-06 10:04:42 +0100, Simon Riggs wrote:
> > > > The issue there is that we continue to issue checkpoints if the only
> > > > activity since the last checkpoint was emitting a standby
> > > > snapshot. That's because:
> > > >
> > >
> > > I agree this is the current situation in 9.4 and 9.5, hence the bug
> > report.
> > >
> > > This no longer occurs with the patches I have proposed. The snapshot is
> > > skipped, so a further checkpoint never triggers.
> >
> > Not if there's a longrunning/idle transaction.
> >
> > Note that skipping the snapshot is actually a *problem* in some
> > cases. As I've brought up upthread, to which you never replied. A
> > xl_running_xacts->xcnt == 0/!overflowed snapshot can be very important
> > for hot standby, because it allows ProcArrayApplyRecoveryInfo() to
> > switch to INITIALIZED state:
> >
> 
> I replied by posting a patch to address your concern, how is that non-reply?

It doesn't address the problem? It's irrelevant that the last snapshot
had 0 xacts, if you start recovery from a later check/restartpoint;
recovery won't process earlier running_xacts records.


> > This obviously doesn't apply to WAL_LEVEL_LOGICAL as is (the if). And it
> > also obviously repeats to log the same snapshot in a system where the
> > state hasn't changed, but where running->xcnt != 0 or nlocks != 0.

> My understanding from your previous comments was that it would be incorrect
> to do that.

I said:
> For one it breaks cleanup with logical decoding which does *NEED* to
> know that nothing is happening. Although only once, not repeatedly.

The salient point is "Although only once, not repeatedly.". Which is
pretty much same thing as for HS; to become consistent after a
checkpoint.


> Not true. I have listened to everything you've said and been patient with
> the high number of mistakes in your replies.

Simon, this is utterly ridiculous. Missing an if in a post-commit
review, of a hastily committed patch, which hasn't previously been
posted for review, is entirely normal.

Greetings,

Andres Freund


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

[COMMITTERS] pgsql: Revert bf08f2292ffca14fd133aa0901d1563b6ecd6894

2016-04-06 Thread Simon Riggs 
Revert bf08f2292ffca14fd133aa0901d1563b6ecd6894

Remove recent changes to logging XLOG_RUNNING_XACTS by request.

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/cac0e36682970ec1276d3da3d3ee37325544a2bb

Modified Files
--
src/backend/postmaster/bgwriter.c |  5 +
src/backend/storage/ipc/standby.c | 21 -
2 files changed, 1 insertion(+), 25 deletions(-)


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

Re: [HACKERS] [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

2016-04-06 Thread Simon Riggs 
On 6 April 2016 at 13:48, Michael Paquier  wrote:

>
> Yeah... We have reached a clear consensus about the way things should
> be done after quite a lot of discussions that has gone for a couple of
> months. And Andres' design on the matter is what is getting approval
> from everybody who has worked toward addressing this issue while
> really taking care of the problem at its root.


Clearly I am not included in your use of the words "we" and "everybody" !

What you mean is that you and Andres agree, so using the word consensus is
not appropriate and certainly not clear. We did all agree on the point that
the earlier fix cannot be backpatched, so if it is as grevious a problem as
described many users will not now benefit.

I will revert my earlier patch now.

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

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

Re: [COMMITTERS] pgsql: Generic Messages for Logical Decoding

2016-04-06 Thread Michael Paquier 
On Wed, Apr 6, 2016 at 6:08 PM, Simon Riggs  wrote:
> Generic Messages for Logical Decoding
>
> API and mechanism to allow generic messages to be inserted into WAL that are
> intended to be read by logical decoding plugins. This commit adds an optional
> new callback to the logical decoding API.
>
> Messages are either text or bytea. Messages can be transactional, or not, and
> are identified by a prefix to allow multiple concurrent decoding plugins.
>
> (Not to be confused with Generic WAL records, which are intended to allow 
> crash
> recovery of extensible objects.)

jacana says boom:
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana=2016-04-06%2012%3A02%3A05

*** 
c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.build/../pgsql/contrib/test_decoding/expected/messages.out
Wed Apr  6 08:02:30 2016
--- 
c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.build/contrib/test_decoding/./regression_output/results/messages.out
Wed Apr  6 08:40:24 2016
***
*** 54,75 

  COMMIT;
  SELECT 'žluťoučký kůň' FROM pg_logical_emit_message(true, 'test',
'žluťoučký kůň');
!?column?
! ---
!  žluťoučký kůň
! (1 row)
!
  SELECT data FROM pg_logical_slot_get_changes('regression_slot',
NULL, NULL, 'force-binary', '0', 'skip-empty-xacts', '1');
   data
! --
   message: transactional: 1 prefix: test, sz: 4 content:msg1
   message: transactional: 0 prefix: test, sz: 4 content:msg2
   message: transactional: 0 prefix: test, sz: 4 content:msg4
   message: transactional: 0 prefix: test, sz: 4 content:msg6
   message: transactional: 1 prefix: test, sz: 4 content:msg5
   message: transactional: 1 prefix: test, sz: 4 content:msg7
!  message: transactional: 1 prefix: test, sz: 19 content:žluťoučký kůň
! (7 rows)

  SELECT 'init' FROM pg_drop_replication_slot('regression_slot');
   ?column?
--- 54,70 

  COMMIT;
  SELECT 'žluťoučký kůň' FROM pg_logical_emit_message(true, 'test',
'žluťoučký kůň');
! ERROR:  character with byte sequence 0xc5 0xa5 in encoding "UTF8"
has no equivalent in encoding "WIN1252"
  SELECT data FROM pg_logical_slot_get_changes('regression_slot',
NULL, NULL, 'force-binary', '0', 'skip-empty-xacts', '1');
  data
! 
   message: transactional: 1 prefix: test, sz: 4 content:msg1
   message: transactional: 0 prefix: test, sz: 4 content:msg2
   message: transactional: 0 prefix: test, sz: 4 content:msg4
   message: transactional: 0 prefix: test, sz: 4 content:msg6
   message: transactional: 1 prefix: test, sz: 4 content:msg5
   message: transactional: 1 prefix: test, sz: 4 content:msg7
! (6 rows)

  SELECT 'init' FROM pg_drop_replication_slot('regression_slot');
   ?column?

I thought we always avoided non-ASCII characters in tests, no?
-- 
Michael


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

Re: [HACKERS] [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

2016-04-06 Thread Simon Riggs 
On 6 April 2016 at 13:27, Andres Freund  wrote:

> On 2016-04-06 13:11:40 +0100, Simon Riggs wrote:
> > On 6 April 2016 at 10:09, Andres Freund  wrote:
> > > On 2016-04-06 10:04:42 +0100, Simon Riggs wrote:
> > > The issue there is that we continue to issue checkpoints if the only
> > > activity since the last checkpoint was emitting a standby
> > > snapshot. That's because:
> > >
> >
> > I agree this is the current situation in 9.4 and 9.5, hence the bug
> report.
> >
> > This no longer occurs with the patches I have proposed. The snapshot is
> > skipped, so a further checkpoint never triggers.
>
> Not if there's a longrunning/idle transaction.
>
> Note that skipping the snapshot is actually a *problem* in some
> cases. As I've brought up upthread, to which you never replied. A
> xl_running_xacts->xcnt == 0/!overflowed snapshot can be very important
> for hot standby, because it allows ProcArrayApplyRecoveryInfo() to
> switch to INITIALIZED state:
>

I replied by posting a patch to address your concern, how is that non-reply?


> > > > What issue is that? Previously you said it must not skip it at all
> for
> > > > logical.
> > >
> > > It's fine to skip the records iff nothing important has happened since
> > > the last time a snapshot has been logged. Again, the proposed approach
> > > allowed to detect that.
>
> > Agreed, both proposals do that.
>
> No, the currently committed patch, even including your proposed followup
> patch, doesn't do that. As you just commented about as quoted above. The
> code currently reads like:
>
> if (wal_level < WAL_LEVEL_LOGICAL)
> {
> LWLockRelease(ProcArrayLock);
>
> /*
>  * Don't bother to log anything if nothing is happening,
> if we are
>  * using archive_timeout > 0 and we didn't overflow
> snapshot last time.
>  *
>  * This ensures that we don't issue an empty WAL record,
> which can
>  * be annoying when used in conjunction with archive
> timeout.
>  */
> if (running->xcnt == 0 &&
> nlocks == 0 &&
> XLogArchiveTimeout > 0 &&
> !last_snapshot_overflowed)
> {
> LWLockRelease(XidGenLock);
> return InvalidXLogRecPtr;
> }
>
> last_snapshot_overflowed = running->subxid_overflow;
> }
>
> This obviously doesn't apply to WAL_LEVEL_LOGICAL as is (the if). And it
> also obviously repeats to log the same snapshot in a system where the
> state hasn't changed, but where running->xcnt != 0 or nlocks != 0.


My understanding from your previous comments was that it would be incorrect
to do that.


> > > > > We now log more WAL with
> > > > > XLogArchiveTimeout > 0 than without.
> > >
> > > > And the problem with that is what?
> > >
> > > That an idle system unnecessarily produces WAL? Waking up disks and
> > > everything?
> > >
> >
> > The OP discussed a problem with archive_timeout > 0. Are you saying we
> > should put in a fix that applies whatever the setting of archive_timeout?
>
> Yes. We should strive to fix the full scope of an issue; not just the
> part complained about.
>

I believe that's what I did. You didn't mention that point earlier, nor do
I now think it important.


> You seem to ignore valid points made against your approach, apparently
> because you dismiss them as "emotive language". Stop.
>

Not true. I have listened to everything you've said and been patient with
the high number of mistakes in your replies.

Calling something a "bandaid" is not a "valid point" against it.

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

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

Re: [HACKERS] [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

2016-04-06 Thread Michael Paquier 
On Wed, Apr 6, 2016 at 9:27 PM, Andres Freund  wrote:
> On 2016-04-06 13:11:40 +0100, Simon Riggs wrote:
>> On 6 April 2016 at 10:09, Andres Freund  wrote:
>> > On 2016-04-06 10:04:42 +0100, Simon Riggs wrote:
>> > The issue there is that we continue to issue checkpoints if the only
>> > activity since the last checkpoint was emitting a standby
>> > snapshot. That's because:
>> >
>>
>> I agree this is the current situation in 9.4 and 9.5, hence the bug report.
>>
>> This no longer occurs with the patches I have proposed. The snapshot is
>> skipped, so a further checkpoint never triggers.
>
> Not if there's a longrunning/idle transaction.
>
> Note that skipping the snapshot is actually a *problem* in some
> cases. As I've brought up upthread, to which you never replied. A
> xl_running_xacts->xcnt == 0/!overflowed snapshot can be very important
> for hot standby, because it allows ProcArrayApplyRecoveryInfo() to
> switch to INITIALIZED state:
> if (standbyState == STANDBY_SNAPSHOT_PENDING)
> {
> /*
>  * If the snapshot isn't overflowed or if its empty we can 
> reset our
>  * pending state and use this snapshot instead.
>  */
> if (!running->subxid_overflow || running->xcnt == 0)
> {
> /*
>  * If we have already collected known assigned xids, 
> we need to
>  * throw them away before we apply the recovery 
> snapshot.
>  */
> KnownAssignedXidsReset();
> standbyState = STANDBY_INITIALIZED;
> }

Yes. Such snapshot allows to initialize more quickly a hot standby...
And that's useful in some cases if the recovery is on a pending
snapshot (bgwriter standby snapshots help a lot with that btw).

>> > > > We now log more WAL with
>> > > > XLogArchiveTimeout > 0 than without.
>> >
>> > > And the problem with that is what?
>> >
>> > That an idle system unnecessarily produces WAL? Waking up disks and
>> > everything?
>> >
>>
>> The OP discussed a problem with archive_timeout > 0. Are you saying we
>> should put in a fix that applies whatever the setting of archive_timeout?
>
> Yes. We should strive to fix the full scope of an issue; not just the
> part complained about.
>
> You seem to ignore valid points made against your approach, apparently
> because you dismiss them as "emotive language". Stop.

Yeah... We have reached a clear consensus about the way things should
be done after quite a lot of discussions that has gone for a couple of
months. And Andres' design on the matter is what is getting approval
from everybody who has worked toward addressing this issue while
really taking care of the problem at its root. The patch, as currently
proposed, is a bandaid, and has been committed at the surprise of
everybody without any discussion.

Please let's revert this patch and really move toward fixing this
problem... Which is a way in short a way to fix the decision-making
for checkpoint skipping.
-- 
Michael


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

Re: [HACKERS] [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

2016-04-06 Thread Andres Freund 
On 2016-04-06 13:11:40 +0100, Simon Riggs wrote:
> On 6 April 2016 at 10:09, Andres Freund  wrote:
> > On 2016-04-06 10:04:42 +0100, Simon Riggs wrote:
> > The issue there is that we continue to issue checkpoints if the only
> > activity since the last checkpoint was emitting a standby
> > snapshot. That's because:
> >
> 
> I agree this is the current situation in 9.4 and 9.5, hence the bug report.
> 
> This no longer occurs with the patches I have proposed. The snapshot is
> skipped, so a further checkpoint never triggers.

Not if there's a longrunning/idle transaction.

Note that skipping the snapshot is actually a *problem* in some
cases. As I've brought up upthread, to which you never replied. A
xl_running_xacts->xcnt == 0/!overflowed snapshot can be very important
for hot standby, because it allows ProcArrayApplyRecoveryInfo() to
switch to INITIALIZED state:
if (standbyState == STANDBY_SNAPSHOT_PENDING)
{
/*
 * If the snapshot isn't overflowed or if its empty we can 
reset our
 * pending state and use this snapshot instead.
 */
if (!running->subxid_overflow || running->xcnt == 0)
{
/*
 * If we have already collected known assigned xids, we 
need to
 * throw them away before we apply the recovery 
snapshot.
 */
KnownAssignedXidsReset();
standbyState = STANDBY_INITIALIZED;
}



> > > What issue is that? Previously you said it must not skip it at all for
> > > logical.
> >
> > It's fine to skip the records iff nothing important has happened since
> > the last time a snapshot has been logged. Again, the proposed approach
> > allowed to detect that.

> Agreed, both proposals do that.

No, the currently committed patch, even including your proposed followup
patch, doesn't do that. As you just commented about as quoted above. The
code currently reads like:

if (wal_level < WAL_LEVEL_LOGICAL)
{
LWLockRelease(ProcArrayLock);

/*
 * Don't bother to log anything if nothing is happening, if we 
are
 * using archive_timeout > 0 and we didn't overflow snapshot 
last time.
 *
 * This ensures that we don't issue an empty WAL record, which 
can
 * be annoying when used in conjunction with archive timeout.
 */
if (running->xcnt == 0 &&
nlocks == 0 &&
XLogArchiveTimeout > 0 &&
!last_snapshot_overflowed)
{
LWLockRelease(XidGenLock);
return InvalidXLogRecPtr;
}

last_snapshot_overflowed = running->subxid_overflow;
}

This obviously doesn't apply to WAL_LEVEL_LOGICAL as is (the if). And it
also obviously repeats to log the same snapshot in a system where the
state hasn't changed, but where running->xcnt != 0 or nlocks != 0.

> > > > We now log more WAL with
> > > > XLogArchiveTimeout > 0 than without.
> >
> > > And the problem with that is what?
> >
> > That an idle system unnecessarily produces WAL? Waking up disks and
> > everything?
> >
> 
> The OP discussed a problem with archive_timeout > 0. Are you saying we
> should put in a fix that applies whatever the setting of archive_timeout?

Yes. We should strive to fix the full scope of an issue; not just the
part complained about.


You seem to ignore valid points made against your approach, apparently
because you dismiss them as "emotive language". Stop.


Greetings,

Andres Freund


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

Re: [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

2016-04-06 Thread Robert Haas 
On Wed, Apr 6, 2016 at 8:02 AM, Simon Riggs  wrote:
>> > We can, if you wish, revert this patch. If we do, we will have nothing,
>> > since I object to the other patch(es).
>>
>> I don't think you have an absolute veto over other patches
>
> Huh? My understanding is I have the same powers as other committers, no more
> but also, no less. If you've seen me claim otherwise, please point where
> that happened.

Uh, that would be in the portion that is still quoted.  "If we do, we
will have nothing, since I object to the other patches."

> Me saying "I object" seems to attract more attention than others for some
> reason. Why is it a discussion point that I object to a patch, whereas if
> you do it, thats fine?

You have every right to object to the patch.  You don't have a right,
nor do I, to say that it won't be committed without your agreement.

> All very strange. People commit changes they didn't post all the time,
> especially on minor bugs such as this.

No, they really don't.

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


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

Re: [HACKERS] [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

2016-04-06 Thread Simon Riggs 
On 6 April 2016 at 10:09, Andres Freund  wrote:

> On 2016-04-06 10:04:42 +0100, Simon Riggs wrote:
> > On 6 April 2016 at 09:45, Andres Freund  wrote:
> >
> > > On 2016-04-06 09:18:54 +0100, Simon Riggs wrote:
> > > > Rather than take that option, I went to the trouble of writing a
> patch
> > > that
> > > > does the same thing but simpler, less invasive and more maintainable.
> > > > Primarily, I did that for you, to avoid you having wasted your time
> and
> > > to
> > > > allow you to backpatch a solution.
> > >
> > > But it doesn't. It doesn't solve the longstanding problem of
> checkpoints
> > > needlessly being repeated due to standby snapshots.
>
> >  I can't see why you say this. I am willing to listen, but this
> > appears to be wrong.
>
> The issue there is that we continue to issue checkpoints if the only
> activity since the last checkpoint was emitting a standby
> snapshot. That's because:
>

I agree this is the current situation in 9.4 and 9.5, hence the bug report.

This no longer occurs with the patches I have proposed. The snapshot is
skipped, so a further checkpoint never triggers.


> The proposed patch allows to fix that in a more principled manner,
> because we can simply check that no "important" records have been
> emitted since the last checkpoint, and skip if that's the case.


I understand the proposal you have made. The patch to implement it is what
I object to; my comments made last Sunday.


> > What issue is that? Previously you said it must not skip it at all for
> > logical.
>
> It's fine to skip the records iff nothing important has happened since
> the last time a snapshot has been logged. Again, the proposed approach
> allowed to detect that.


Agreed, both proposals do that.


> > > We now log more WAL with
> > > XLogArchiveTimeout > 0 than without.
>
> > And the problem with that is what?
>
> That an idle system unnecessarily produces WAL? Waking up disks and
> everything?
>

The OP discussed a problem with archive_timeout > 0. Are you saying we
should put in a fix that applies whatever the setting of archive_timeout?

Are we concerned that a master sends a small amount of data every few
minutes to a standby it is connected to? I hadn't read that in the thread.

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

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

Re: [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

2016-04-06 Thread Simon Riggs 
On 6 April 2016 at 12:24, Robert Haas  wrote:

> On Wed, Apr 6, 2016 at 4:18 AM, Simon Riggs  wrote:
> >> FWIW, I vote also for reverting this patch. This has been committed
> >> without any real discussions..
> >
> > Michael, its a shame to hear you say that, so let me give full context.
> >
> > The patches under review in the CF are too invasive and not worth the
> > trouble for such a minor problem. After full review, I would simply
> reject
> > those patches (already discussed on list).
> >
> > Rather than take that option, I went to the trouble of writing a patch
> that
> > does the same thing but simpler, less invasive and more maintainable.
> > Primarily, I did that for you, to avoid you having wasted your time and
> to
> > allow you to backpatch a solution.
> >
> > We can, if you wish, revert this patch. If we do, we will have nothing,
> > since I object to the other patch(es).
>
> I don't think you have an absolute veto over other patches


Huh? My understanding is I have the same powers as other committers, no
more but also, no less. If you've seen me claim otherwise, please point
where that happened.

Me saying "I object" seems to attract more attention than others for some
reason. Why is it a discussion point that I object to a patch, whereas if
you do it, thats fine?

, though you
> certainly have the right to object, and you certainly don't have to
> commit them yourself.  But even more than that, the fact that you
> don't like those other patches does not mean that you can commit
> something without discussion.  Even if every argument you are making
> here is correct, which I'm not sure about, other people obviously
> don't think so.  That stuff should be worked out, as far as possible,
> in pre-commit review, which is only possible when you post the patch
> before committing it.  I think it is fine to commit things
> occasionally without posting them ahead of time if they are obviously
> uncontroversial, but that isn't the case here.
>

All very strange. People commit changes they didn't post all the time,
especially on minor bugs such as this.

Obviously if I knew that it would be controversial I would not have done
it. We are discussing it now, so I don't see any problem.

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

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

Re: [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

2016-04-06 Thread Robert Haas 
On Wed, Apr 6, 2016 at 4:18 AM, Simon Riggs  wrote:
>> FWIW, I vote also for reverting this patch. This has been committed
>> without any real discussions..
>
> Michael, its a shame to hear you say that, so let me give full context.
>
> The patches under review in the CF are too invasive and not worth the
> trouble for such a minor problem. After full review, I would simply reject
> those patches (already discussed on list).
>
> Rather than take that option, I went to the trouble of writing a patch that
> does the same thing but simpler, less invasive and more maintainable.
> Primarily, I did that for you, to avoid you having wasted your time and to
> allow you to backpatch a solution.
>
> We can, if you wish, revert this patch. If we do, we will have nothing,
> since I object to the other patch(es).

I don't think you have an absolute veto over other patches, though you
certainly have the right to object, and you certainly don't have to
commit them yourself.  But even more than that, the fact that you
don't like those other patches does not mean that you can commit
something without discussion.  Even if every argument you are making
here is correct, which I'm not sure about, other people obviously
don't think so.  That stuff should be worked out, as far as possible,
in pre-commit review, which is only possible when you post the patch
before committing it.  I think it is fine to commit things
occasionally without posting them ahead of time if they are obviously
uncontroversial, but that isn't the case here.

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


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

Re: [HACKERS] [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

2016-04-06 Thread Andres Freund 
On 2016-04-06 10:04:42 +0100, Simon Riggs wrote:
> On 6 April 2016 at 09:45, Andres Freund  wrote:
> 
> > On 2016-04-06 09:18:54 +0100, Simon Riggs wrote:
> > > Rather than take that option, I went to the trouble of writing a patch
> > that
> > > does the same thing but simpler, less invasive and more maintainable.
> > > Primarily, I did that for you, to avoid you having wasted your time and
> > to
> > > allow you to backpatch a solution.
> >
> > But it doesn't. It doesn't solve the longstanding problem of checkpoints
> > needlessly being repeated due to standby snapshots.

>  I can't see why you say this. I am willing to listen, but this
> appears to be wrong.

The issue there is that we continue to issue checkpoints if the only
activity since the last checkpoint was emitting a standby
snapshot. That's because:
/*
 * If this isn't a shutdown or forced checkpoint, and we have not 
inserted
 * any XLOG records since the start of the last checkpoint, skip the
 * checkpoint.  The idea here is to avoid inserting duplicate 
checkpoints
 * when the system is idle. That wastes log space, and more importantly 
it
 * exposes us to possible loss of both current and previous checkpoint
 * records if the machine crashes just as we're writing the update.
 * (Perhaps it'd make even more sense to checkpoint only when the 
previous
 * checkpoint record is in a different xlog page?)
 *
 * If the previous checkpoint crossed a WAL segment, however, we create
 * the checkpoint anyway, to have the latest checkpoint fully contained 
in
 * the new segment. This is for a little bit of extra robustness: it's
 * better if you don't need to keep two WAL segments around to recover 
the
 * checkpoint.
 */
if ((flags & (CHECKPOINT_IS_SHUTDOWN | CHECKPOINT_END_OF_RECOVERY |
  CHECKPOINT_FORCE)) == 0)
{
if (prevPtr == ControlFile->checkPointCopy.redo &&
prevPtr / XLOG_SEG_SIZE == curInsert / XLOG_SEG_SIZE)
{
WALInsertLockRelease();
LWLockRelease(CheckpointLock);
END_CRIT_SECTION();
return;
}
}
doesn't trigger anymore.

The proposed patch allows to fix that in a more principled manner,
because we can simply check that no "important" records have been
emitted since the last checkpoint, and skip if that's the case.


> What issue is that? Previously you said it must not skip it at all for
> logical.

It's fine to skip the records iff nothing important has happened since
the last time a snapshot has been logged. Again, the proposed approach
allowed to detect that.


> > We now log more WAL with
> > XLogArchiveTimeout > 0 than without.

> And the problem with that is what?

That an idle system unnecessarily produces WAL? Waking up disks and
everything?


> I'm not much concerned with what emotive language you choose to support
> your arguments

Err.  You're side-tracking the discussion.

Greetings,

Andres Freund


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

[COMMITTERS] pgsql: Generic Messages for Logical Decoding

2016-04-06 Thread Simon Riggs 
Generic Messages for Logical Decoding

API and mechanism to allow generic messages to be inserted into WAL that are
intended to be read by logical decoding plugins. This commit adds an optional
new callback to the logical decoding API.

Messages are either text or bytea. Messages can be transactional, or not, and
are identified by a prefix to allow multiple concurrent decoding plugins.

(Not to be confused with Generic WAL records, which are intended to allow crash
recovery of extensible objects.)

Author: Petr Jelinek and Andres Freund
Reviewers: Artur Zakirov, Tomas Vondra, Simon Riggs
Discussion: 5685f999.6010...@2ndquadrant.com

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/3fe3511d05127cc024b221040db2eeb352e7d716

Modified Files
--
contrib/test_decoding/Makefile  |   2 +-
contrib/test_decoding/expected/ddl.out  |  21 ++--
contrib/test_decoding/expected/messages.out |  79 
contrib/test_decoding/sql/ddl.sql   |   3 +-
contrib/test_decoding/sql/messages.sql  |  25 +
contrib/test_decoding/test_decoding.c   |  18 
doc/src/sgml/func.sgml  |  45 +
doc/src/sgml/logicaldecoding.sgml   |  38 
src/backend/access/rmgrdesc/Makefile|   6 +-
src/backend/access/rmgrdesc/logicalmsgdesc.c|  41 
src/backend/access/transam/rmgr.c   |   1 +
src/backend/replication/logical/Makefile|   2 +-
src/backend/replication/logical/decode.c|  46 +
src/backend/replication/logical/logical.c   |  38 
src/backend/replication/logical/logicalfuncs.c  |  27 ++
src/backend/replication/logical/message.c   |  87 +
src/backend/replication/logical/reorderbuffer.c | 121 
src/backend/replication/logical/snapbuild.c |  19 
src/bin/pg_xlogdump/.gitignore  |  21 +---
src/bin/pg_xlogdump/rmgrdesc.c  |   1 +
src/include/access/rmgrlist.h   |   1 +
src/include/catalog/pg_proc.h   |   4 +
src/include/replication/logicalfuncs.h  |   2 +
src/include/replication/message.h   |  41 
src/include/replication/output_plugin.h |  13 +++
src/include/replication/reorderbuffer.h |  22 +
src/include/replication/snapbuild.h |   2 +
27 files changed, 693 insertions(+), 33 deletions(-)


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

Re: [HACKERS] [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

2016-04-06 Thread Simon Riggs 
On 6 April 2016 at 09:45, Andres Freund  wrote:

> On 2016-04-06 09:18:54 +0100, Simon Riggs wrote:
> > Rather than take that option, I went to the trouble of writing a patch
> that
> > does the same thing but simpler, less invasive and more maintainable.
> > Primarily, I did that for you, to avoid you having wasted your time and
> to
> > allow you to backpatch a solution.
>
> But it doesn't. It doesn't solve the longstanding problem of checkpoints
> needlessly being repeated due to standby snapshots.


 I can't see why you say this. I am willing to listen, but this
appears to be wrong.


> It doesn't fix the
> issue for for wal_level=logical.


What issue is that? Previously you said it must not skip it at all for
logical.


> We now log more WAL with
> XLogArchiveTimeout > 0 than without.
>

And the problem with that is what?


> The other was an architectural fix, this is a selectively applied
> bandaid.
>

It was an attempt at an architectural fix, which went wrong by being too
much code and too invasive for such a minor issue.

I'm not much concerned with what emotive language you choose to support
your arguments, but I am concerned about clear, maintainable code and I
object to the previous patch.

There are other problems worthy of our attention and I will attend to those
now.

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

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

Re: [HACKERS] [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

2016-04-06 Thread Andres Freund 
On 2016-04-06 09:18:54 +0100, Simon Riggs wrote:
> Rather than take that option, I went to the trouble of writing a patch that
> does the same thing but simpler, less invasive and more maintainable.
> Primarily, I did that for you, to avoid you having wasted your time and to
> allow you to backpatch a solution.

But it doesn't. It doesn't solve the longstanding problem of checkpoints
needlessly being repeated due to standby snapshots. It doesn't fix the
issue for for wal_level=logical. We now log more WAL with
XLogArchiveTimeout > 0 than without.

The other was an architectural fix, this is a selectively applied
bandaid.

Andres


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

[COMMITTERS] pgsql: Support multiple synchronous standby servers.

2016-04-06 Thread Fujii Masao 
Support multiple synchronous standby servers.

Previously synchronous replication offered only the ability to confirm
that all changes made by a transaction had been transferred to at most
one synchronous standby server.

This commit extends synchronous replication so that it supports multiple
synchronous standby servers. It enables users to consider one or more
standby servers as synchronous, and increase the level of transaction
durability by ensuring that transaction commits wait for replies from
all of those synchronous standbys.

Multiple synchronous standby servers are configured in
synchronous_standby_names which is extended to support new syntax of
'num_sync ( standby_name [ , ... ] )', where num_sync specifies
the number of synchronous standbys that transaction commits need to
wait for replies from and standby_name is the name of a standby
server.

The syntax of 'standby_name [ , ... ]' which was used in 9.5 or before
is also still supported. It's the same as new syntax with num_sync=1.

This commit doesn't include "quorum commit" feature which was discussed
in pgsql-hackers. Synchronous standbys are chosen based on their priorities.
synchronous_standby_names determines the priority of each standby for
being chosen as a synchronous standby. The standbys whose names appear
earlier in the list are given higher priority and will be considered as
synchronous. Other standby servers appearing later in this list
represent potential synchronous standbys.

The regression test for multiple synchronous standbys is not included
in this commit. It should come later.

Authors: Sawada Masahiko, Beena Emerson, Michael Paquier, Fujii Masao
Reviewed-By: Kyotaro Horiguchi, Amit Kapila, Robert Haas, Simon Riggs,
Amit Langote, Thomas Munro, Sameer Thakur, Suraj Kharage, Abhijit Menon-Sen,
Rajeev Rastogi

Many thanks to the various individuals who were involved in
discussing and developing this feature.

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/989be0810dffd08b54e1caecec0677608211c339

Modified Files
--
doc/src/sgml/config.sgml  |  49 ++-
doc/src/sgml/high-availability.sgml   |  76 +++-
src/backend/Makefile  |   4 +-
src/backend/replication/.gitignore|   2 +
src/backend/replication/Makefile  |  11 +-
src/backend/replication/syncrep.c | 549 --
src/backend/replication/syncrep_gram.y|  86 
src/backend/replication/syncrep_scanner.l | 144 +++
src/backend/replication/walsender.c   |  19 +-
src/backend/utils/misc/guc.c  |   2 +-
src/backend/utils/misc/postgresql.conf.sample |   2 +-
src/include/replication/syncrep.h |  31 +-
src/tools/msvc/Mkvcbuild.pm   |   2 +-
13 files changed, 806 insertions(+), 171 deletions(-)


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

Re: [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

2016-04-06 Thread Simon Riggs 
On 5 April 2016 at 01:18, Michael Paquier  wrote:

> On Mon, Apr 4, 2016 at 4:50 PM, Andres Freund  wrote:
> > On 2016-04-04 08:44:47 +0100, Simon Riggs wrote:
> >> That patch does exactly the same thing as the patch you prefer, just
> >> does it differently;
> >
> > No, it doesn't; as explained above.
>

I think these few changes are all we need. (attached)


FWIW, I vote also for reverting this patch. This has been committed
> without any real discussions..
>

Michael, its a shame to hear you say that, so let me give full context.

The patches under review in the CF are too invasive and not worth the
trouble for such a minor problem. After full review, I would simply reject
those patches (already discussed on list).

Rather than take that option, I went to the trouble of writing a patch that
does the same thing but simpler, less invasive and more maintainable.
Primarily, I did that for you, to avoid you having wasted your time and to
allow you to backpatch a solution.

We can, if you wish, revert this patch. If we do, we will have nothing,
since I object to the other patch(es).

My recommendation is that we apply the attached patch and leave it there.

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

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


avoid_running_xacts_log.v1plus.patch
Description: Binary data

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