date:20141127

On Thu, Nov 27, 2014 at 9:39 PM, Sameer Kumar 
wrote:

> Can we tweak pg_rewind to take differential backups in PostgreSQL?
> I was wondering can we hack the pg_rewind code to print the details of the
> file which have been modified compared to a target server. The list output
> can then be used for taking differential backups.
>
> Or perhaps we can add an option/switch in pg_rewind --action
>
> --action=print ---> would print the files which have changed
> --action=sync ---> would sync them
> --action=copy ---> with this option I can specify an additional optino
> --target-dir where I can copy the files which have changed
>

This discussion is not really adapted on hackers as pg_rewind is not
included in Postgres core code. Please let's discuss your proposal there.
Btw, pg_rewind is not aimed to be used as a tool for a backup facility. You
may find easier to use existing backup solutions instead, or help out with
an in-core solution. There was actually not so long ago a patch to
implement in-core differential backups, with a part called profiles able to
fetch back the list of files modified since a given LSN point (only an
improvement of the core feature though as that's not mandatory to make the
machinery work). The patch has not been added to any of the recent commit
fests though.
Regards,
-- 
Michael

Re: [HACKERS] [REVIEW] Re: Compression of full-page-writes

On Fri, Nov 28, 2014 at 1:30 PM, Rahila Syed  wrote:
> I have attached the changes separately as changes.patch.

Yes thanks.
FWIW, I noticed those things as well when going through the code again
this morning for my tests. Note as well that the declaration of
doPageCompression at the top of xlog.c was an integer while it should
have been a boolean.
Regards,
-- 
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] [REVIEW] Re: Compression of full-page-writes

On Fri, Nov 28, 2014 at 3:48 PM, Michael Paquier
 wrote:
> Configuration
> ==
> 3) HEAD + fdw = on
> start LSN: 0/1BC8
> stop LSN:
> difference:
Wrong copy/paste:
stop LSN = 0/8FA8
difference = 1872MB
tps = 2057.344827 (including connections establishing)
tps = 2057.468800 (excluding connections establishing)
-- 
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] [REVIEW] Re: Compression of full-page-writes

So, I have been doing some more tests with this patch. I think the
compression numbers are in line with the previous tests.

Configuration
==

3 sets are tested:
- HEAD (a5eb85e) + fpw = on
- patch + fpw = on
- patch + fpw = compress
With the following configuration:
shared_buffers=512MB
checkpoint_segments=1024
checkpoint_timeout = 5min
fsync=off

WAL quantity
===
pgbench -s 30 -i (455MB of data)
pgbench -c 32 -j 32 -t 45000 -M prepared (roughly 11 min of run on
laptop, two checkpoints kick in)

1) patch + fdw = compress
tps = 2086.893948 (including connections establishing)
tps = 2087.031543 (excluding connections establishing)
start LSN: 0/1990
stop LSN: 0/49F73D78
difference: 783MB

2) patch + fdw = on
start LSN: 0/1B90
stop LSN: 0/8F4E1BD0
difference: 1861 MB
tps = 2106.812454 (including connections establishing)
tps = 2106.953329 (excluding connections establishing)

3) HEAD + fdw = on
start LSN: 0/1BC8
stop LSN:
difference:

WAL replay performance
===
Then tested replay time of a standby after replaying WAL files
generated by previous pgbench runs and by tracking "redo start" and
"redo stop". Goal here is to check for the same amount of activity how
much block decompression plays on replay. The replay includes the
pgbench initialization phase.

1) patch + fdw = compress
1-1) Try 1.
2014-11-28 14:09:27.287 JST: LOG:  redo starts at 0/3000380
2014-11-28 14:10:19.836 JST: LOG:  redo done at 0/49F73E18
Result: 52.549
1-2) Try 2.
2014-11-28 14:15:04.196 JST: LOG:  redo starts at 0/3000380
2014-11-28 14:15:56.238 JST: LOG:  redo done at 0/49F73E18
Result: 52.042
1-3) Try 3
2014-11-28 14:20:27.186 JST: LOG:  redo starts at 0/3000380
2014-11-28 14:21:19.350 JST: LOG:  redo done at 0/49F73E18
Result: 52.164
2) patch + fdw = on
2-1) Try 1
2014-11-28 14:42:54.670 JST: LOG:  redo starts at 0/3000750
2014-11-28 14:43:56.221 JST: LOG:  redo done at 0/8F4E1BD0
Result: 61.5s
2-2) Try 2
2014-11-28 14:46:03.198 JST: LOG:  redo starts at 0/3000750
2014-11-28 14:47:03.545 JST: LOG:  redo done at 0/8F4E1BD0
Result: 60.3s
2-3) Try 3
2014-11-28 14:50:26.896 JST: LOG:  redo starts at 0/3000750
2014-11-28 14:51:30.950 JST: LOG:  redo done at 0/8F4E1BD0
Result: 64.0s
3) HEAD + fdw = on
3-1) Try 1
2014-11-28 15:21:48.153 JST: LOG:  redo starts at 0/3000750
2014-11-28 15:22:53.864 JST: LOG:  redo done at 0/8FA8
Result: 65.7s
3-2) Try 2
2014-11-28 15:27:16.271 JST: LOG:  redo starts at 0/3000750
2014-11-28 15:28:20.677 JST: LOG:  redo done at 0/8FA8
Result: 64.4s
3-3) Try 3
2014-11-28 15:36:30.434 JST: LOG:  redo starts at 0/3000750
2014-11-28 15:37:33.208 JST: LOG:  redo done at 0/8FA8
Result: 62.7s

So we are getting an equivalent amount of WAL when compression is not
enabled with both HEAD and the patch, aka a reduction of 55% at
constant number of transactions with pgbench. The difference seems to
be some noise. Note that basically as the patch adds a uint16 in
XLogRecordBlockImageHeader to store the length of the block compressed
and achieve a double level of compression (1st level being the removal
of the page hole), the records are 2 bytes longer per block image, it
does not seem to be much a problem in those tests. Regarding the WAL
replay, compressed blocks need extra CPU for decompression in exchange
of having less WAL to replay in quantity, this is actually reducing by
~15% the replay time, so the replay plays in favor of putting the load
on the CPU. Also, I haven't seen any difference with or without the
patch when compression is disabled.

Updated patches attached, I found a couple of issues with the code
this morning (issues more or less pointed out as well by Rahila
earlier) before running those tests.
Regards,

Regards,
-- 
Michael


0001-Move-pg_lzcompress.c-to-src-common.patch.gz
Description: GNU Zip compressed data


0002-Support-compression-for-full-page-writes-in-WAL.patch.gz
Description: GNU Zip compressed data

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

[HACKERS] Allocation in critical section after node exits archive recovery

Hi all,

While performing some archive recovery tests I found the following
assertion failure once standby gets out of archive recovery (HEAD at
a5eb85e):
(lldb)
* thread #1: tid = 0x, 0x7fff8faaf282
libsystem_kernel.dylib`__pthread_kill + 10, stop reason = signal
SIGSTOP
  * frame #0: 0x7fff8faaf282 libsystem_kernel.dylib`__pthread_kill + 10
frame #1: 0x7fff888774c3 libsystem_pthread.dylib`pthread_kill + 90
frame #2: 0x7fff898acb73 libsystem_c.dylib`abort + 129
frame #3: 0x000107106029
postgres`ExceptionalCondition(conditionName=0x00010725770b,
errorType=0x000107219b83, fileName=0x0001072c4c84,
lineNumber=555) + 137 at assert.c:54
frame #4: 0x000107140ec5
postgres`MemoryContextCreate(tag=T_AllocSetContext, size=192,
methods=0x0001072fc7b0, parent=0x7fca6a405710,
name=0x000107257e1b) + 117 at mcxt.c:555
frame #5: 0x00010713e6c7
postgres`AllocSetContextCreate(parent=0x7fca6a405710,
name=0x000107257e1b, minContextSize=0, initBlockSize=8192,
maxBlockSize=8388608) + 71 at aset.c:444
frame #6: 0x000106c51449 postgres`InitXLogInsert + 73 at
xloginsert.c:869
frame #7: 0x000106c450ee postgres`InitXLOGAccess + 190 at xlog.c:7281
frame #8: 0x000106c43e7c postgres`LocalSetXLogInsertAllowed +
76 at xlog.c:7133
frame #9: 0x000106c445dd postgres`CreateCheckPoint(flags=38) +
1165 at xlog.c:7693
frame #10: 0x000106ebf935 postgres`CheckpointerMain + 1989 at
checkpointer.c:502
frame #11: 0x000106c5a0c3
postgres`AuxiliaryProcessMain(argc=2, argv=0x7fff5909bb30) + 1907
at bootstrap.c:426
frame #12: 0x000106ecf696
postgres`StartChildProcess(type=CheckpointerProcess) + 358 at
postmaster.c:5109
frame #13: 0x000106ece229
postgres`sigusr1_handler(postgres_signal_arg=30) + 265 at
postmaster.c:4795
frame #14: 0x7fff91468f1a libsystem_platform.dylib`_sigtramp + 26
frame #15: 0x7fff8faaf3f7 libsystem_kernel.dylib`__select + 11
frame #16: 0x000106ecd717 postgres`PostmasterMain(argc=3,
argv=0x7fca6a4056a0) + 5479 at postmaster.c:1220
frame #17: 0x000106e11fc5 postgres`main(argc=3,
argv=0x7fca6a4056a0) + 773 at main.c:220

Logs:
2014-11-28 15:28:20.733 JST: LOG:  selected new timeline ID: 2
cp: /Users/ioltas/archive/5432/0001.history: No such file or
directory
2014-11-28 15:28:20.776 JST: LOG:  archive recovery complete
TRAP: FailedAssertion("!(CritSectionCount == 0)", File: "mcxt.c", Line: 555)

This area of the code has been changed recently by the WAL API patch.
Note that I haven't looked yet at implementing a fix.
Regards,
-- 
Michael


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

[HACKERS] Testing DDL deparsing support

2014-11-27 Thread Ian Barwick

DDL deparsing is a feature that allows collection of DDL commands as they are
executed in a server, in some flexible, complete, and fully-contained format
that allows manipulation, storage, and transmission. This feature has several
use cases; the two best known ones are DDL replication and DDL auditing.

We have came up with a design that uses a JSON structure to store commands.
It is similar to the C sprintf() call in spirit: there is a base format
string, which is a generic template for each command type, and contains
placeholders that represent the variable parts of the command. The values for
the placeholders in each specific command are members of the JSON object. A
helper function is provided that expands the format string and replaces the
placeholders with the values, and returns the SQL command as text. This
design lets the user operate on the JSON structure in either a read-only
fashion (for example to block table creation if the names don't satisfy a
certain condition), or by modifying it (for example, to change the schema name
so that tables are created in different schemas when they are replicated to
some remote server).

This design is mostly accepted by the community. The one sticking point is
testing: how do we ensure that the JSON representation we have created
correctly deparses back into a command that has the same effect as the
original command. This was expressed by Robert Haas:
http://www.postgresql.org/message-id/CA+TgmoZ=vzrijmxlkqi_v0jg4k4leapmwusc6rwxs5mquxu...@mail.gmail.com

The problem cannot be solved by a standard regression test module which runs a
bunch of previously-defined commands and verifies the output. We need not
only check the output for the commands as they exist today, but also we need
to ensure that this does not get broken as future patches modify the existing
commands as well as create completely new commands.

The challenge here is to create a new testing framework that ensures the DDL
deparsing module will be maintained by future hackers as the DDL grammar is
modified.

What and How to Test

Our goal should be that patch authors run "make check-world" in their patched
copies and notice that the DDL deparse test is failing; they can then modify
deparse_utility.c to add support for the new commands, which should in general
be pretty straightforward. This way, maintaining deparsing code would be part
of new patches just like we require pg_dump support and documentation for new
features.

It would not work to require patch authors to add their new commands to a new
pg_regress test file, because most would not be aware of the need, or they
would just forget to do it, and patches would be submitted and possibly even
committed without any realization of the breakage caused.

There are two things we can rely on: standard regression tests, and pg_dump.

Standard regression tests are helpful because patch authors include new test
cases in the patches that stress their new options or commands. This new test
framework needs to be something that internally runs the regression tests and
exercises DDL deparsing as the regression tests execute DDL. That would mean
that new commands and options would automatically be deparse-tested by our new
framework as soon as patch authors add standard regress support.

One thing is first-grade testing, that is ensure that the deparsed version of
a DDL command can be executed at all, for if the deparsed version throws an
error, it's immediately obvious that the deparse code is bogus. This is
because we know the original command did not throw an error: otherwise, the
deparse code would not have run at all, because ddl_command_end triggers are
only executed once the original command has completed execution. So
first-grade testing ensures that no trivial bugs are present.

But there's second-grade verification as well: is the object produced by the
deparsed version identical to the one produced by the original command? One
trivial but incomplete approach is to run the command, then save the deparsed
version; run the deparsed version, and deparse that one too; compare both.
The problem with this approach is that if the deparse code is omitting some
clause (say it omits IN TABLESPACE in a CREATE TABLE command), then both
deparsed versions would contain the same bug yet they would compare equal.
Therefore this approach is not good enough.

The best idea we have so far to attack second-grade testing is to trust
pg_dump to expose differences: accumulate commands as they run in the
regression database, the run the deparsed versions in a different database;
then pg_dump both databases and compare the dumped outputs.

Proof-of-concept

We have now implemented this as a proof-of-concept; the code is available
in the deparse branch at:

http://git.postgresql.org/gitweb/?p=2ndquadrant_bdr.git

a diff is attached for reference, but relies on the deparsing functionality
available in the deparse branch

Re: [HACKERS] [REVIEW] Re: Compression of full-page-writes

2014-11-27 Thread Rahila Syed

>if (!fullPageWrites)
>{
>   WALInsertLockAcquireExclusive();
>Insert->fullPageWrites = fullPageWrites;
>WALInsertLockRelease();
>}
>

As fullPageWrites is not a boolean isnt it better to change the if
condition as fullPageWrites == FULL_PAGE_WRITES_OFF? As it is done in the
if condition above, this seems to be a miss.

>doPageWrites = (Insert->fullPageWrites || Insert->forcePageWrites);

IIUC, doPageWrites is true when fullPageWrites is either 'on' or
'compress'
Considering Insert -> fullPageWrites is an int now, I think its better to
explicitly write the above as ,

doPageWrites = (Insert -> fullPageWrites != FULL_PAGE_WRITES_OFF ||
Insert->forcePageWrites)

The patch attached has the above changes. Also, it initializes
doPageCompression in InitXLOGAccess as per earlier discussion.

I have attached the changes separately as changes.patch.

Thank you,

Rahila Syed


changes.patch
Description: Binary data


0002-Support-compression-for-full-page-writes-in-WAL.patch
Description: Binary data

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

Re: [HACKERS] why is PG_AUTOCONF_FILENAME is pg_config_manual.h?

2014-11-27 Thread Amit Kapila

On Thu, Nov 27, 2014 at 8:28 PM, Peter Eisentraut  wrote:
>
> Surely that's not a value that we expect users to be able to edit.  Is
> pg_config_manual.h just abused as a place that's included everywhere?
>

Changing PG_AUTOCONF_FILENAME even by developer would
lead to loss of useful configuration settings (if any done by user via
ALTER SYSTEM) and on starting server user will see some
log message indicating that it has skipped processing configuration
file.  I think the impact of changing this define is not so severe that
someone has to do initdb, however it's non-ignorable impact.

The file header of pg_config_manual.h seems to suggest that this
file contains such parameters.

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

Re: [HACKERS] no test programs in contrib

2014-11-27 Thread Kouhei Kaigai

> -Original Message-
> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Josh Berkus
> Sent: Friday, November 28, 2014 5:48 AM
> To: Alvaro Herrera; Pg Hackers
> Subject: Re: [HACKERS] no test programs in contrib
> 
> On 11/24/2014 05:49 AM, Alvaro Herrera wrote:
> > test_parser (a toy text search parser, added in 2007) dummy_seclabel
> > (for SECURITY LABEL regression testing, added Sept 2010) worker_spi
> > (for bgworkers, added Dec 2012) test_shm_mq (test program for shared
> > memory queues, added Jan 2014) test_decoding (test program for logical
> > decoding, added March 2014)
> 
> So test_decoding is fairly useful for users demonstrating that decoding
> works, especially if they're also testing an external decoding module and
> are unsure of where their replication problem is located, or what's wrong
> with their HBA settings.  For that reason it's important that test_decoding
> be available via OS packages, which would give me some reluctance to move
> it out of /contrib.
> 
> dummy_seclabel might serve the same purpose for users who are having issues
> with SEPostgres etc.  I don't know enough about it ...
> Stephen/Kaigai?
> 
Its original purpose is to run regression test on the platform without
selinux. So, it does not intend to use the dummy_seclabel for something
useful except for regression test.

Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei 

> --
> Josh Berkus
> PostgreSQL Experts Inc.
> http://pgexperts.com
> 
> 
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

-- 
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] pg_regress and --dbname option / multiple databases

2014-11-27 Thread Ian Barwick




On 28/11/14 00:02, Andrew Dunstan wrote:


On 11/27/2014 04:12 AM, Ian Barwick wrote:

Hi

pg_regress provides the command line option "--dbname",
which is described in the help output thusly:

   --dbname=DBuse database DB (default "regression")

It does however accept multiple comma separated names
and will create a database for each name provided,
but AFAICS only ever uses the first database in the list.

Is there a reason for this I'm not seeing?


Most of the code is shared between the main regression suite and ecpg's

> regression suit. The latter uses multiple databases, I believe.

Aha, indeed it does. Thanks for the clarification.


Regards

Ian Barwick

--
 Ian Barwick   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, 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] [pgsql-packagers] Palle Girgensohn's ICU patch

2014-11-27 Thread Tatsuo Ishii

> Another issue is that (AFAIK) ICU doesn't support any non-Unicode
> encodings, which means that a build supporting *only* ICU collations is a
> nonstarter IMO.  So we really need a way to deal with both system and ICU
> collations, and treating the latter as a separate subset of pg_collation
> seems like a decent way to do that.  (ISTR some discussion about forcibly
> converting strings in other encodings to Unicode to compare them, but
> I sure don't want to do that.  I think it'd be saner just to mark the
> ICU collations as only compatible with UTF8 database encoding.)

+1. Forcing only Unicode collation is totally unacceptable.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
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-packagers] Palle Girgensohn's ICU patch

2014-11-27 Thread Peter Geoghegan

On Thu, Nov 27, 2014 at 7:03 AM, Tom Lane  wrote:
> +1 ... this seems like a nice end-run around the backwards compatibility
> problem.
>
> Another issue is that (AFAIK) ICU doesn't support any non-Unicode
> encodings, which means that a build supporting *only* ICU collations is a
> nonstarter IMO.  So we really need a way to deal with both system and ICU
> collations, and treating the latter as a separate subset of pg_collation
> seems like a decent way to do that.  (ISTR some discussion about forcibly
> converting strings in other encodings to Unicode to compare them, but
> I sure don't want to do that.  I think it'd be saner just to mark the
> ICU collations as only compatible with UTF8 database encoding.)

I would like to see ICU become the defacto standard set of collations,
with support for *versioning*, in the same way that UTF-8 might be
considered the defacto standard encoding.

It seems likely that we'll want to store sort keys (strxfrm() blobs)
in indexes at some point in the future. I now believe that that's more
problematic than just using strcoll() in B-Tree support function 1.
Although that isn't the most compelling reason to pursue ICU support.
-- 
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] no test programs in contrib

2014-11-27 Thread Alvaro Herrera

Tom Lane wrote:

> I'm not too happy with that approach, because packagers are going to
> tend to think they should package any files installed by install-world.
> The entire point of this change, IMO, is that the test modules should
> *not* get installed, certainly not by normal install targets.  Being
> consistent with the existing contrib packaging is exactly not what we
> want.
> 
> Maybe we should only allow check-world to run these tests, and not
> installcheck-world?  That's kind of annoying, but what you are
> doing now seems to defeat the purpose altogether.

Hadn't thought of the packaging angle of this.  I don't think packagers
really are as dumb as you suggest, but anyway implementing this idea
turned out to be simpler than I expected; here's a preliminary patch.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/GNUmakefile.in b/GNUmakefile.in
index 8dbbcee..7c3c657 100644
--- a/GNUmakefile.in
+++ b/GNUmakefile.in
@@ -32,7 +32,7 @@ install:
 install-docs:
 	$(MAKE) -C doc install
 
-$(call recurse,install-world,doc src config contrib src/test/modules,install)
+$(call recurse,install-world,doc src config contrib,install)
 install-world:
 	+@echo "PostgreSQL, contrib, and documentation installation complete."
 
@@ -66,7 +66,7 @@ check check-tests: all
 check check-tests installcheck installcheck-parallel installcheck-tests:
 	$(MAKE) -C src/test/regress $@
 
-$(call recurse,check-world,src/test src/pl src/interfaces/ecpg contrib src/bin,check)
+$(call recurse,check-world,src/test src/pl src/interfaces/ecpg contrib src/bin src/test/modules,check)
 
 $(call recurse,installcheck-world,src/test src/pl src/interfaces/ecpg contrib src/bin,installcheck)
 
diff --git a/src/test/Makefile b/src/test/Makefile
index 5d997b8..ff061c1 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -14,4 +14,10 @@ include $(top_builddir)/src/Makefile.global
 
 SUBDIRS = regress isolation modules
 
-$(recurse)
+standard_recurse_targets := $(filter-out installcheck install, $(standard_targets))
+
+# We want to recurse to all subdirs for all standard targets, except that
+# installcheck and install should not recurse into the subdirectory "modules".
+$(call recurse,$(standard_recurse_targets))
+$(call recurse,installcheck,regress isolation)
+$(call recurse,install,regress isolation)

-- 
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] no test programs in contrib

Josh Berkus  writes:
> So test_decoding is fairly useful for users demonstrating that decoding
> works, especially if they're also testing an external decoding module
> and are unsure of where their replication problem is located, or what's
> wrong with their HBA settings.  For that reason it's important that
> test_decoding be available via OS packages, which would give me some
> reluctance to move it out of /contrib.

If we follow that reasoning we'll end up removing nothing from contrib.
There is no reason that end users should need to be performing such
testing; anyone who does have reason to do it will have a source tree
at hand.

> dummy_seclabel might serve the same purpose for users who are having
> issues with SEPostgres etc.  I don't know enough about it ...

And as for dummy_seclabel, the same applies in spades, considering
that the number of users of SEPostgres can probably be counted without
running out of fingers.

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] no test programs in contrib

2014-11-27 Thread Josh Berkus

On 11/24/2014 05:49 AM, Alvaro Herrera wrote:
> test_parser (a toy text search parser, added in 2007)
> dummy_seclabel (for SECURITY LABEL regression testing, added Sept 2010)
> worker_spi (for bgworkers, added Dec 2012)
> test_shm_mq (test program for shared memory queues, added Jan 2014)
> test_decoding (test program for logical decoding, added March 2014)

So test_decoding is fairly useful for users demonstrating that decoding
works, especially if they're also testing an external decoding module
and are unsure of where their replication problem is located, or what's
wrong with their HBA settings.  For that reason it's important that
test_decoding be available via OS packages, which would give me some
reluctance to move it out of /contrib.

dummy_seclabel might serve the same purpose for users who are having
issues with SEPostgres etc.  I don't know enough about it ...
Stephen/Kaigai?

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

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

[HACKERS] OverrideSearchPathMatchesCurrent performance improvement

The other thing that emerged from
http://www.postgresql.org/message-id/flat/CAOR=d=3j1U_q-zf8+jUx1hkx8ps+N8pm=EUTqyFdJ5ov=+f...@mail.gmail.com
is that for workloads like this, OverrideSearchPathMatchesCurrent is
a bottleneck: it's part of the code path needed to re-use a cached plan,
and this example is doing a lot of that.  The original implementation
of that function followed the KISS principle, since I thought it probably
wouldn't be a bottleneck.  Now that that's proven wrong, I offer the
attached reimplementation, which saves about 9% of overall runtime on
Scott's example by avoiding palloc/pfree traffic.  It knows a lot more
about the relation of override search paths to active search paths than
it did before; but the adjacent functions know these things too, so it
doesn't seem like much of a loss from a maintainability standpoint.

regards, tom lane

diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 911f015..7a06332 100644
*** a/src/backend/catalog/namespace.c
--- b/src/backend/catalog/namespace.c
*** CopyOverrideSearchPath(OverrideSearchPat
*** 3145,3164 
  bool
  OverrideSearchPathMatchesCurrent(OverrideSearchPath *path)
  {
! 	/* Easiest way to do this is GetOverrideSearchPath() and compare */
! 	bool		result;
! 	OverrideSearchPath *cur;
  
! 	cur = GetOverrideSearchPath(CurrentMemoryContext);
! 	if (path->addCatalog == cur->addCatalog &&
! 		path->addTemp == cur->addTemp &&
! 		equal(path->schemas, cur->schemas))
! 		result = true;
! 	else
! 		result = false;
! 	list_free(cur->schemas);
! 	pfree(cur);
! 	return result;
  }
  
  /*
--- 3145,3188 
  bool
  OverrideSearchPathMatchesCurrent(OverrideSearchPath *path)
  {
! 	ListCell   *lc,
! 			   *lcp;
  
! 	recomputeNamespacePath();
! 
! 	/* We scan down the activeSearchPath to see if it matches the input. */
! 	lc = list_head(activeSearchPath);
! 
! 	/* If path->addTemp, first item should be my temp namespace. */
! 	if (path->addTemp)
! 	{
! 		if (lc && lfirst_oid(lc) == myTempNamespace)
! 			lc = lnext(lc);
! 		else
! 			return false;
! 	}
! 	/* If path->addCatalog, next item should be pg_catalog. */
! 	if (path->addCatalog)
! 	{
! 		if (lc && lfirst_oid(lc) == PG_CATALOG_NAMESPACE)
! 			lc = lnext(lc);
! 		else
! 			return false;
! 	}
! 	/* We should now be looking at the activeCreationNamespace. */
! 	if (activeCreationNamespace != (lc ? lfirst_oid(lc) : InvalidOid))
! 		return false;
! 	/* The remainder of activeSearchPath should match path->schemas. */
! 	foreach(lcp, path->schemas)
! 	{
! 		if (lc && lfirst_oid(lc) == lfirst_oid(lcp))
! 			lc = lnext(lc);
! 		else
! 			return false;
! 	}
! 	if (lc)
! 		return false;
! 	return true;
  }
  
  /*

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

Re: [HACKERS] [REVIEW] Re: Compression of full-page-writes

On Thu, Nov 27, 2014 at 11:59 PM, Michael Paquier
 wrote:
> On Thu, Nov 27, 2014 at 11:42 PM, Andres Freund  
> wrote:
>> One thing Heikki brought up somewhere, which I thought to be a good
>> point, was that it might be worthwile to forget about compressing FDWs
>> themselves, and instead compress entire records when they're large. I
>> think that might just end up being rather beneficial, both from a code
>> simplicity and from the achievable compression ratio.
> Indeed, that would be quite simple to do. Now determining an ideal cap
> value is tricky. We could always use a GUC switch to control that but
> that seems sensitive to set, still we could have a recommended value
> in the docs found after looking at some average record size using the
> regression tests.

Thinking more about that, it would be difficult to apply the
compression for all records because of the buffer that needs to be
pre-allocated for compression, or we would need to have each code path
creating a WAL record able to forecast the size of this record, and
then adapt the size of the buffer before entering a critical section.
Of course we could still apply this idea for records within a given
windows size.
Still, the FPW compression does not have those concerns. A buffer used
for compression is capped by BLCKSZ for a single block, and nblk *
BLCKSZ if blocks are grouped for compression.
Feel free to comment if I am missing smth obvious.
Regards,
-- 
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] no test programs in contrib

Alvaro Herrera  writes:
> I have also changed things so that:

> 1. test modules are not installed by "make install", not checked by
> "make installcheck", not checked by "make check".

> 2. test modules are checked by "make check-world" (this is consistent
> with handling of contrib).

> 3. test modules are checked by "make installcheck-world" (this is
> consistent with handling of contrib)

> 4. test modules are installed by "make install-world".  This is
> consistent with contrib, and it's necessary so that "make
> installcheck-world" passes.

I'm not too happy with that approach, because packagers are going to
tend to think they should package any files installed by install-world.
The entire point of this change, IMO, is that the test modules should
*not* get installed, certainly not by normal install targets.  Being
consistent with the existing contrib packaging is exactly not what we
want.

Maybe we should only allow check-world to run these tests, and not
installcheck-world?  That's kind of annoying, but what you are
doing now seems to defeat the purpose altogether.

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] no test programs in contrib

2014-11-27 Thread Alvaro Herrera

Peter Eisentraut wrote:
> On 11/26/14 9:27 AM, Alvaro Herrera wrote:
> > I haven't done anything about documentation.  I thought a new chapter
> > after "Additional Supplied Modules", perhaps entitled "Additional Sample
> > Modules" would be appropriate.
> 
> I would remove the SGML files and put simple README files into each
> directory.

They are so small that it makes sense to do it like that.  Here's a
patch for this.

I have also changed things so that:

1. test modules are not installed by "make install", not checked by
"make installcheck", not checked by "make check".

2. test modules are checked by "make check-world" (this is consistent
with handling of contrib).

3. test modules are checked by "make installcheck-world" (this is
consistent with handling of contrib)

4. test modules are installed by "make install-world".  This is
consistent with contrib, and it's necessary so that "make
installcheck-world" passes.


I moved the contents from SGML files into READMEs, and removed the
references from other SGML files, turning them into
contrib/ instead (these are release-9.4 and so on, which is
why I reference the old locations.  I assume release-9.5 will mention
the moves).

There's some untested new code in vcregress.pl, but nothing else
about msvc has been done.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/GNUmakefile.in b/GNUmakefile.in
index 69e0824..8dbbcee 100644
--- a/GNUmakefile.in
+++ b/GNUmakefile.in
@@ -32,7 +32,7 @@ install:
 install-docs:
 	$(MAKE) -C doc install
 
-$(call recurse,install-world,doc src config contrib,install)
+$(call recurse,install-world,doc src config contrib src/test/modules,install)
 install-world:
 	+@echo "PostgreSQL, contrib, and documentation installation complete."
 
diff --git a/contrib/Makefile b/contrib/Makefile
index b37d0dd..efee109 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -16,7 +16,6 @@ SUBDIRS = \
 		dblink		\
 		dict_int	\
 		dict_xsyn	\
-		dummy_seclabel	\
 		earthdistance	\
 		file_fdw	\
 		fuzzystrmatch	\
@@ -50,13 +49,9 @@ SUBDIRS = \
 		spi		\
 		tablefunc	\
 		tcn		\
-		test_decoding	\
-		test_parser	\
-		test_shm_mq	\
 		tsearch2	\
 		unaccent	\
-		vacuumlo	\
-		worker_spi
+		vacuumlo
 
 ifeq ($(with_openssl),yes)
 SUBDIRS += sslinfo
diff --git a/contrib/pg_upgrade/test.sh b/contrib/pg_upgrade/test.sh
index 7bbd2c7..41614b2 100644
--- a/contrib/pg_upgrade/test.sh
+++ b/contrib/pg_upgrade/test.sh
@@ -65,6 +65,7 @@ if [ "$1" = '--install' ]; then
 	"$MAKE" -s -C ../.. install DESTDIR="$temp_install"
 	"$MAKE" -s -C ../pg_upgrade_support install DESTDIR="$temp_install"
 	"$MAKE" -s -C . install DESTDIR="$temp_install"
+	"$MAKE" -s -C ../../src/test/modules install DESTDIR="$temp_install"
 
 	# platform-specific magic to find the shared libraries; see pg_regress.c
 	LD_LIBRARY_PATH=$libdir:$LD_LIBRARY_PATH
@@ -143,7 +144,14 @@ set -x
 
 "$oldbindir"/initdb -N
 "$oldbindir"/pg_ctl start -l "$logdir/postmaster1.log" -o "$POSTMASTER_OPTS" -w
-if "$MAKE" -C "$oldsrc" installcheck; then
+"$MAKE" -C "$oldsrc" installcheck
+make_installcheck_status=$?
+if [ $make_installcheck_status -eq 0 ]; then
+	"$MAKE" -C "$oldsrc"/src/test/modules installcheck
+	make_installcheck_status=$?
+fi
+
+if [ $make_installcheck_status -eq 0 ]; then
 	pg_dumpall -f "$temp_root"/dump1.sql || pg_dumpall1_status=$?
 	if [ "$newsrc" != "$oldsrc" ]; then
 		oldpgversion=`psql -A -t -d regression -c "SHOW server_version_num"`
@@ -165,12 +173,9 @@ if "$MAKE" -C "$oldsrc" installcheck; then
 		sed "s;$oldsrc;$newsrc;g" "$temp_root"/dump1.sql.orig >"$temp_root"/dump1.sql
 	fi
 else
-	make_installcheck_status=$?
-fi
-"$oldbindir"/pg_ctl -m fast stop
-if [ -n "$make_installcheck_status" ]; then
 	exit 1
 fi
+"$oldbindir"/pg_ctl -m fast stop
 if [ -n "$psql_fix_sql_status" ]; then
 	exit 1
 fi
diff --git a/doc/src/sgml/contrib.sgml b/doc/src/sgml/contrib.sgml
index ec68f10..8836b0c 100644
--- a/doc/src/sgml/contrib.sgml
+++ b/doc/src/sgml/contrib.sgml
@@ -113,7 +113,6 @@ CREATE EXTENSION module_name FROM unpackaged;
  &dblink;
  &dict-int;
  &dict-xsyn;
- &dummy-seclabel;
  &earthdistance;
  &file-fdw;
  &fuzzystrmatch;
@@ -140,9 +139,6 @@ CREATE EXTENSION module_name FROM unpackaged;
  &sslinfo;
  &tablefunc;
  &tcn;
- &test-decoding;
- &test-parser;
- &test-shm-mq;
  &tsearch2;
  &unaccent;
  &uuid-ossp;
diff --git a/doc/src/sgml/dummy-seclabel.sgml b/doc/src/sgml/dummy-seclabel.sgml
deleted file mode 100644
index d064705..000
--- a/doc/src/sgml/dummy-seclabel.sgml
+++ /dev/null
@@ -1,74 +0,0 @@
-
-
-
- dummy_seclabel
-
- 
-  dummy_seclabel
- 
-
- 
-  The dummy_seclabel module exists only to support regression
-  testing of the SECURITY LABEL statement.  It is not intended
-  to be used in production.
- 
-
- 
-  Rationale
-
-  
-   The SECURITY LABEL statement allows the user to assign security
-   labels to database objects; however, security labels can only be assigned
-   when spe

[HACKERS] Marginal performance improvement: replace bms_first_member loops

Another thing that came out of the discussion at
http://www.postgresql.org/message-id/flat/CAOR=d=3j1U_q-zf8+jUx1hkx8ps+N8pm=EUTqyFdJ5ov=+f...@mail.gmail.com
was that there was a significant amount of palloc/pfree traffic blamable
on the bms_first_member() loop in plpgsql's setup_param_list().  I've
been experimenting with a variant of bms_first_member() called
bms_next_member(), which doesn't modify the input bitmapset and therefore
removes the need to make a working copy when iterating over the members
of a set.

In isolation, bms_next_member() is fractionally slower than
bms_first_member() because it has to do a bit more shifting-and-masking,
but of course we more than win that back from eliminating a palloc/pfree
cycle.  It's also worth noting that in principle, a bms_first_member()
loop is O(N^2) for large sets because it scans from the start of the
set each time; but I doubt this is much of an issue in practice, because
the bitmapsets we work with just aren't very large.  (I did some
microbenchmarking and found that if one ignores the palloc overhead
question, a bms_next_member loop is a tad slower up to about four words
in the bitmapset, and faster beyond that because the rescans start to
make a difference.  But four words would be 128 bits and very very few
bitmapsets in PG would have more members than that.)

The attached proposed patch adds bms_next_member() and replaces
bms_first_member() calls where it seemed to make sense.  I've had a
hard time measuring much speed difference for this patch in isolation,
but in principle it should be less code and less cycles.  It also seems
safer and more natural to not use destructive looping techniques.

regards, tom lane

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 76bda73..7dcedd1 100644
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
*** postgresPlanForeignModify(PlannerInfo *r
*** 1198,1212 
  	}
  	else if (operation == CMD_UPDATE)
  	{
- 		Bitmapset  *tmpset = bms_copy(rte->modifiedCols);
  		AttrNumber	col;
  
! 		while ((col = bms_first_member(tmpset)) >= 0)
  		{
  			col += FirstLowInvalidHeapAttributeNumber;
  			if (col <= InvalidAttrNumber)		/* shouldn't happen */
  elog(ERROR, "system-column update is not supported");
  			targetAttrs = lappend_int(targetAttrs, col);
  		}
  	}
  
--- 1198,1213 
  	}
  	else if (operation == CMD_UPDATE)
  	{
  		AttrNumber	col;
  
! 		col = -1;
! 		while ((col = bms_next_member(rte->modifiedCols, col)) >= 0)
  		{
  			col += FirstLowInvalidHeapAttributeNumber;
  			if (col <= InvalidAttrNumber)		/* shouldn't happen */
  elog(ERROR, "system-column update is not supported");
  			targetAttrs = lappend_int(targetAttrs, col);
+ 			col -= FirstLowInvalidHeapAttributeNumber;
  		}
  	}
  
diff --git a/contrib/sepgsql/dml.c b/contrib/sepgsql/dml.c
index bb82c0d..44c067d 100644
*** a/contrib/sepgsql/dml.c
--- b/contrib/sepgsql/dml.c
*** static Bitmapset *
*** 94,100 
  fixup_inherited_columns(Oid parentId, Oid childId, Bitmapset *columns)
  {
  	AttrNumber	attno;
- 	Bitmapset  *tmpset;
  	Bitmapset  *result = NULL;
  	char	   *attname;
  	int			index;
--- 94,99 
*** fixup_inherited_columns(Oid parentId, Oi
*** 105,112 
  	if (parentId == childId)
  		return columns;
  
! 	tmpset = bms_copy(columns);
! 	while ((index = bms_first_member(tmpset)) > 0)
  	{
  		attno = index + FirstLowInvalidHeapAttributeNumber;
  
--- 104,111 
  	if (parentId == childId)
  		return columns;
  
! 	index = -1;
! 	while ((index = bms_next_member(columns, index)) > 0)
  	{
  		attno = index + FirstLowInvalidHeapAttributeNumber;
  
*** fixup_inherited_columns(Oid parentId, Oi
*** 128,139 
  			elog(ERROR, "cache lookup failed for attribute %s of relation %u",
   attname, childId);
  
! 		index = attno - FirstLowInvalidHeapAttributeNumber;
! 		result = bms_add_member(result, index);
  
  		pfree(attname);
  	}
- 	bms_free(tmpset);
  
  	return result;
  }
--- 127,137 
  			elog(ERROR, "cache lookup failed for attribute %s of relation %u",
   attname, childId);
  
! 		result = bms_add_member(result,
! attno - FirstLowInvalidHeapAttributeNumber);
  
  		pfree(attname);
  	}
  
  	return result;
  }
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index c499486..436b0fa 100644
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
*** ExecCheckRTEPerms(RangeTblEntry *rte)
*** 547,553 
  	AclMode		remainingPerms;
  	Oid			relOid;
  	Oid			userid;
- 	Bitmapset  *tmpset;
  	int			col;
  
  	/*
--- 547,552 
*** ExecCheckRTEPerms(RangeTblEntry *rte)
*** 614,621 
  	return false;
  			}
  
! 			tmpset = bms_copy(rte->selectedCols);
! 			while ((col = bms_first_member(tmpset)) >= 0)
  			{
  /* remove the column number offset */
  col += FirstLow

Re: [HACKERS] The problems of PQhost()

2014-11-27 Thread Noah Misch

On Fri, Nov 28, 2014 at 03:11:06AM +0900, Fujii Masao wrote:
> On Thu, Nov 27, 2014 at 12:38 PM, Noah Misch  wrote:
> > Sure.  I'll first issue "git revert 9f80f48", then apply the attached patch.
> > Since libpq ignores a hostaddr parameter equal to the empty string, this
> > implementation does likewise.  Apart from that, I anticipate behavior
> > identical to today's code.
> 
> +fprintf(stderr, _("out of memory\n"));
> 
> psql_error() should be used instead of fprintf()?

I copied what pg_malloc() would do.  Either way seems reasonable.

> +if (host == NULL)/* can't happen */
> 
> Is this comment true? ISTM that "host" can be NULL when the default socket
> directory is used, i.e., neither host nor hostaddr are supplied by the user.

Right; I will adjust accordingly.  Thanks.


-- 
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: [BUGS] BUG #12071: Stat collector went crasy (50MB/s constant writes)

2014-11-27 Thread Tomas Vondra

Moving to pgsql-hackers, as that's a more appropriate place for this
discussion.

On 27.11.2014 11:26, Maxim Boguk wrote:
> 
> 
> FWIW, I got curious and checked why we decided not to implement this
> while reworking the stats in 9.3, as keeping an is_dirty flag seems as a
> rather straightforward and simple optimization.
> 
> Turns out it's actually considerably more complex, because one of the
> sources of statistics updates are (surprise surprise) autovacuum
> workers. So the whole flow may look like this (in >= 9.3):
> 
>1) launcher requests a fresh stats file (dbs list only)
>2) worker is started for a particular DB (by launcher)
>3) the worker requests a stats file (with details for the DB)
> 
> 
> 
> Now for (nearly) idle databases worker do nothing and simple exit in 99.9%.

While it may sound trivial, I believe it's actually more complicated
than just adding a 'is_dirty' flag.

The first question is what 'nearly idle' means. Is that a number of
modified rows? A number of tuples read from tables? Because this would
affect autovacuum processes as well as regular backends (i.e. reading
from pg_stat_user_tables might return stale data). I don't see a
definition that's obviously correct :-(

Secondly, even a 'nearly idle' database needs a vacuum/analyze from time
to time, so there needs to be some logic to detect that. So the logic of
updating is_dirty flag (or maybe reading the value) needs to consider that.

The only way I can think of is duplicating the 'needs vacuum / analyze'
detection into the collector, and setting the flag to 'true' when
there's at least one object in need of autovacuum. This is however much
more complex than a simple is_dirty flag, and I'm not sure it's really
worth it ...

> And if there a lot of idle/low-active db's in cluster - is_dirty
> tracking would be beneficial on both pre 9.3 and after 9.3 versions (and
> in 9.3+ it will be especially effective because id_dirty tracked per-db
> basis).

I don't think so. First, there are practical issues making it way more
complex than what you assume (see my explanation in the preceding text).

Second, let's do a bit of math. With 5MB of stats, 10 databases and 1s
naptime, the old (pre-9.3) implementation writes ~50MB/s. With the new
implementation, this drops to ~5MB/s, because only stats for that
particular database need to be written.

That's a huge improvement. And if even such I/O load is an issue (which
I'd say is unlikely), then move the pg_stat_tmp directory to a tmpfs
filesystem. Problem solved.

Also, you're the first person with such low naptime value (which is
really the culprit here), so most people really don't have such issues.
And I'm still not convinced this is the best way to deal with the
workload you described (tables that grow / get deleted quickly).

I'm not going to spend more time hacking on this, as I doubt it's worth
the effort. Feel free to propose a patch, of course.

regards
Tomas

-- 
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] The problems of PQhost()

On Thu, Nov 27, 2014 at 12:38 PM, Noah Misch  wrote:
> On Tue, Nov 25, 2014 at 09:53:10PM +0900, Fujii Masao wrote:
>> On Tue, Nov 25, 2014 at 12:37 PM, Noah Misch  wrote:
>> > On Wed, Jan 22, 2014 at 11:48:26PM +0900, Fujii Masao wrote:
>> >> (3) PQhost() cannot return the hostaddr.
>> >
>> >> We can fix the problem (3) by changing PQhost() so that it also
>> >> returns the hostaddr. But this change might break the existing
>> >> application using PQhost(). So, I added new libpq function PQhostaddr()
>> >> which returns the hostaddr, and changed \conninfo so that it reports
>> >> correct connection information by using both PQhost() and PQhostaddr().
>> >
>> >> + 
>> >> +  
>> >> +   PQhostaddr
>> >> +   
>> >> +PQhostaddr
>> >> +   
>> >> +  
>> >> +
>> >> +  
>> >> +   
>> >> +Returns the server numeric IP address or host name of the 
>> >> connection.
>> >> + 
>> >> + char *PQhostaddr(const PGconn *conn);
>> >> + 
>> >> +   
>> >> +  
>> >> + 
>> >
>> > From reading this documentation, I assumed this function would return a
>> > non-empty value for every TCP connection.  After all, every TCP connection 
>> > has
>> > a peer IP address.  In fact, PQhostaddr() returns the raw value of the
>> > "hostaddr" connection parameter, whether from a libpq function argument or
>> > from the PGHOSTADDR environment variable.  (If the parameter and 
>> > environment
>> > variable are absent, it returns NULL.  Adding "hostaddr=" to the connection
>> > string makes it return the empty string.)  A caller wanting the specific 
>> > raw
>> > value of a parameter could already use PQconninfo().  I suspect this new
>> > function will confuse more than help.  What do you think of reverting it 
>> > and
>> > having \conninfo use PQconninfo() to discover any "hostaddr" value?
>>
>> Sounds reasonable to me. Are you planning to implement and commit the patch?
>
> Sure.  I'll first issue "git revert 9f80f48", then apply the attached patch.
> Since libpq ignores a hostaddr parameter equal to the empty string, this
> implementation does likewise.  Apart from that, I anticipate behavior
> identical to today's code.

+fprintf(stderr, _("out of memory\n"));

psql_error() should be used instead of fprintf()?

+if (host == NULL)/* can't happen */

Is this comment true? ISTM that "host" can be NULL when the default socket
directory is used, i.e., neither host nor hostaddr are supplied by the user.

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] why is PG_AUTOCONF_FILENAME is pg_config_manual.h?

On Thu, Nov 27, 2014 at 11:58 PM, Peter Eisentraut  wrote:
> Surely that's not a value that we expect users to be able to edit.  Is
> pg_config_manual.h just abused as a place that's included everywhere?
>
> (I suggest utils/guc.h as a better place.)

+1

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] psql \watch always ignores \pset null

On Wed, Nov 19, 2014 at 3:24 PM, Tom Lane  wrote:
> Will Leinweber  writes:
>> On Tue, Nov 18, 2014 at 9:54 PM, Tom Lane  wrote:
>>> Fujii Masao  writes:
 Is there any reason why \watch must ignore \pset null setting?
>
>>> Hmmm ... the comment offers a reasonable argument for forcing pager = 0,
>>> but I agree the nullPrint change is not adequately explained.
>>> Will, do you remember why you did that?
>
>> I tracked down the individual commit[1] from my history where I added
>> that. What I added there is very similar to sections in
>> src/bin/psql/describe.c. I can't remember specifically my reasoning
>> then, but it's likely I copied the patterns there while getting things
>> working.
>> I do still think it's important to remove the pager, but the nullPrint
>> is probably a mistake.
>
> I took a quick look and noted that the other places where nullPrint is
> summarily forced to null are for \d and similar queries.  For those,
> the code can reasonably have an opinion about what the presentation should
> be like, since it knows what SQL query it's issuing.  That argument surely
> doesn't apply to \watch, so I'm in agreement with Fujii that it'd be
> better to respect the user's \pset setting.

Thanks! I've just fixed this problem.

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] pg_isready: Missing translation macros.

On Thu, Nov 27, 2014 at 9:32 PM, Mats Erik Andersson  
wrote:
> Hello there,
>
> the text response of pg_isready is hard coded in English.
> These short snippets really ought to be localized as well.

Thanks for the patch! Committed.

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] pg_dump/pg_restore seem broken on hamerkop

On Sun, Oct 26, 2014 at 08:29:19PM -0400, Tom Lane wrote:
> I wrote:
> > Hm.  %z ought not be locale-dependent ... however, it has a bigger
> > problem, which is that it's a C99-ism.  It's not there in SUSv2,
> > which is our normal baseline for what's portable.  I think we need
> > to get rid of that.  %Z should be portable.
> 
> > (Is it possible that Windows' strftime() reads %z as doing something
> > other than what C99 says?)
> 
> A bit of googling leads me to Microsoft reference material saying that
> their strftime treats %z and %Z alike.  So in point of fact, the
> assumption underlying commit ad5d46a4494b0b48 was flat out wrong.
> Switching to %z doesn't get you out of the problem noted in the
> comments it removed:
> 
> /*
>  * We don't print the timezone on Win32, because the names are long and
>  * localized, which means they may contain characters in various random
>  * encodings; this has been seen to cause encoding errors when reading the
>  * dump script.
>  */
> 
> I'm going to go revert most of that commit and make the code like it
> was before:

Thanks.  The major goal of the patch was to get a timezone designation
in there, and you have done that were possible, which is non-Windows. 
Your C comment clearly explained why Windows is a problem, and
centralized the code.

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

  + Everyone has their own god. +


-- 
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-packagers] Palle Girgensohn's ICU patch

Greg Stark  writes:
> Hm. Actually the pg_collation catalog might give a handy way out for the
> issue of being inconsistent with the system collation. We could support
> both sets of collations and let the user select an ICU collation or system
> collation at runtime.

+1 ... this seems like a nice end-run around the backwards compatibility
problem.

Another issue is that (AFAIK) ICU doesn't support any non-Unicode
encodings, which means that a build supporting *only* ICU collations is a
nonstarter IMO.  So we really need a way to deal with both system and ICU
collations, and treating the latter as a separate subset of pg_collation
seems like a decent way to do that.  (ISTR some discussion about forcibly
converting strings in other encodings to Unicode to compare them, but
I sure don't want to do that.  I think it'd be saner just to mark the
ICU collations as only compatible with UTF8 database encoding.)

regards, tom lane

PS: I've removed pgsql-packagers from the cc, this thread is no
longer relevant to them.

-- 
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] pg_regress and --dbname option / multiple databases

2014-11-27 Thread Andrew Dunstan



On 11/27/2014 04:12 AM, Ian Barwick wrote:

Hi

pg_regress provides the command line option "--dbname",
which is described in the help output thusly:

   --dbname=DBuse database DB (default "regression")

It does however accept multiple comma separated names
and will create a database for each name provided,
but AFAICS only ever uses the first database in the list.

Is there a reason for this I'm not seeing?





Most of the code is shared between the main regression suite and ecpg's 
regression suit. The latter uses multiple databases, I believe.


cheers

andrew



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

Re: [HACKERS] [REVIEW] Re: Compression of full-page-writes

On Thu, Nov 27, 2014 at 11:42 PM, Andres Freund  wrote:
> On 2014-11-27 13:00:57 +0900, Michael Paquier wrote:
>> This is backward-incompatible in the fact that forcibly-written FPWs
>> would be compressed all the time, even if FPW is set to off. The
>> documentation of the previous patches also mentioned that images are
>> compressed only if this parameter value is switched to compress.
>
> err, "backward incompatible"? I think it's quite useful to allow
> compressing newpage et. al records even if FPWs aren't required for the
> hardware.
Incorrect words. This would enforce a new behavior on something that's
been like that for ages even if we have a switch to activate it.

> One thing Heikki brought up somewhere, which I thought to be a good
> point, was that it might be worthwile to forget about compressing FDWs
> themselves, and instead compress entire records when they're large. I
> think that might just end up being rather beneficial, both from a code
> simplicity and from the achievable compression ratio.
Indeed, that would be quite simple to do. Now determining an ideal cap
value is tricky. We could always use a GUC switch to control that but
that seems sensitive to set, still we could have a recommended value
in the docs found after looking at some average record size using the
regression tests.
-- 
Michael


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

[HACKERS] why is PG_AUTOCONF_FILENAME is pg_config_manual.h?

2014-11-27 Thread Peter Eisentraut

Surely that's not a value that we expect users to be able to edit.  Is
pg_config_manual.h just abused as a place that's included everywhere?

(I suggest utils/guc.h as a better place.)


-- 
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] Fillfactor for GIN indexes

2014-11-27 Thread Cédric Villemain

Le jeudi 27 novembre 2014 23:33:11 Michael Paquier a écrit :
> On Fri, Nov 21, 2014 at 2:12 PM, Michael Paquier
> 
>  wrote:
> > I am adding that to the commit fest of December.
> 
> Here are updated patches. Alvaro notified me about an inconsistent
> comment.

what are the benefits of this patch ? (maybe you had some test case or a 
benchmark ?)
-- 
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation

signature.asc
Description: This is a digitally signed message part.

Re: [HACKERS] Proposal : REINDEX SCHEMA

On Thu, Nov 27, 2014 at 8:18 PM, Sawada Masahiko  wrote:
> On Thu, Nov 27, 2014 at 1:07 PM, Michael Paquier
>  wrote:
>> On Thu, Nov 27, 2014 at 12:55 AM, Sawada Masahiko  
>> wrote:
>>> +1 to define new something object type and remove do_user and do_system.
>>> But if we add OBJECT_SYSTEM to ObjectType data type,
>>> system catalogs are OBJECT_SYSTEM as well as OBJECT_TABLE.
>>> It's a bit redundant?
>> Yes, kind of. That's a superset of a type of relations, aka a set of
>> catalog tables. If you find something cleaner to propose, feel free.
>
> I thought we can add new struct like ReindexObjectType which has
> REINDEX_OBJECT_TABLE,
> REINDEX_OBJECT_SYSTEM and so on. It's similar to GRANT syntax.
Check.

 Another thing, ReindexDatabaseOrSchema should be renamed to ReindexObject.
 So, I think that we need to think a bit more here. We are not far from
 smth that could be committed, so marking as "Waiting on Author" for
 now. Thoughts?
>>>
>>> Is the table also kind of "object"?
>> Sorry, I am not sure I follow you here. Indexes and tables have
>> already their relkind set in ReindexStmt, and I think that we're fine
>> to continue letting them go in their own reindex code path for now.
>
> It was not enough, sorry.
> I mean that there is already ReindexTable() function.
> if we renamed ReindexObject, I would feel uncomfortable feeling.
> Because table is also kind of "object".
Check.

If you get that done, I'll have an extra look at it and then let's
have a committer look at it.
Regards,
-- 
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] [REVIEW] Re: Compression of full-page-writes

2014-11-27 Thread Andres Freund

On 2014-11-27 13:00:57 +0900, Michael Paquier wrote:
> On Wed, Nov 26, 2014 at 8:27 PM, Syed, Rahila  wrote:
> > Don't we need to initialize doPageCompression  similar to doPageWrites in 
> > InitXLOGAccess?
> Yep, you're right. I missed this code path.
> 
> > Also , in the earlier patches compression was set 'on' even when fpw GUC is 
> > 'off'. This was to facilitate compression of FPW which are forcibly written 
> > even when fpw GUC is turned off.
> >  doPageCompression in this patch is set to true only if value of fpw GUC is 
> > 'compress'. I think its better to compress forcibly written full page 
> > writes.
> Meh? (stealing a famous quote).

> This is backward-incompatible in the fact that forcibly-written FPWs
> would be compressed all the time, even if FPW is set to off. The
> documentation of the previous patches also mentioned that images are
> compressed only if this parameter value is switched to compress.

err, "backward incompatible"? I think it's quite useful to allow
compressing newpage et. al records even if FPWs aren't required for the
hardware.

One thing Heikki brought up somewhere, which I thought to be a good
point, was that it might be worthwile to forget about compressing FDWs
themselves, and instead compress entire records when they're large. I
think that might just end up being rather beneficial, both from a code
simplicity and from the achievable compression ratio.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, 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] Fillfactor for GIN indexes

On Fri, Nov 21, 2014 at 2:12 PM, Michael Paquier
 wrote:
> I am adding that to the commit fest of December.
Here are updated patches. Alvaro notified me about an inconsistent comment.
-- 
Michael
From eda0730d991f8b4dfbacc4d7a953ec5bff8b2ffe Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Fri, 21 Nov 2014 13:40:11 +0900
Subject: [PATCH 1/2] Fix flag marking GIN index as being built for new entries

This was somewhat missing in the current implementation, and leaded
to problems for code that needed special handling with fresh indexes
being built. Note that this does not impact current code as there are
no such operations being done yet but it may be a problem if in the
future a bug fix needs to make this distinction.
---
 src/backend/access/gin/gininsert.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/backend/access/gin/gininsert.c b/src/backend/access/gin/gininsert.c
index c1ad0fd..c6d8b40 100644
--- a/src/backend/access/gin/gininsert.c
+++ b/src/backend/access/gin/gininsert.c
@@ -191,6 +191,7 @@ ginEntryInsert(GinState *ginstate,
 		buildStats->nEntries++;
 
 	ginPrepareEntryScan(&btree, attnum, key, category, ginstate);
+	btree.isBuild = (buildStats != NULL);
 
 	stack = ginFindLeafPage(&btree, false);
 	page = BufferGetPage(stack->buffer);
-- 
2.2.0

From eeb6f4d4cd4dcc713c7c4a6526fa9d4cf49c0262 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Fri, 21 Nov 2014 14:08:54 +0900
Subject: [PATCH 2/2] Support fillfactor for GIN indexes

Users can call this new storage parameter to fill in the entry and
leaf pages of a newly-built index as wanted. Fillfactor range varies
between 20 and 100.
---
 doc/src/sgml/ref/create_index.sgml |  4 ++--
 src/backend/access/common/reloptions.c |  9 +
 src/backend/access/gin/gindatapage.c   | 22 ++
 src/backend/access/gin/ginentrypage.c  | 20 +++-
 src/backend/access/gin/ginutil.c   |  3 ++-
 src/include/access/gin_private.h   |  3 +++
 6 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 6b2ee28..c0ba24a 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -294,8 +294,8 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] 
 The optional WITH clause specifies storage
 parameters for the index.  Each index method has its own set of allowed
-storage parameters.  The B-tree, hash, GiST and SP-GiST index methods all
-accept this parameter:
+storage parameters.  The B-tree, hash, GIN, GiST and SP-GiST index methods
+all accept this parameter:

 

diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index c16b38e..7137ba9 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -15,6 +15,7 @@
 
 #include "postgres.h"
 
+#include "access/gin_private.h"
 #include "access/gist_private.h"
 #include "access/hash.h"
 #include "access/htup_details.h"
@@ -133,6 +134,14 @@ static relopt_int intRelOpts[] =
 	},
 	{
 		{
+			"fillfactor",
+			"Packs gin index pages only to this percentage",
+			RELOPT_KIND_GIN
+		},
+		GIN_DEFAULT_FILLFACTOR, GIN_MIN_FILLFACTOR, 100
+	},
+	{
+		{
 			"autovacuum_vacuum_threshold",
 			"Minimum number of tuple updates or deletes prior to vacuum",
 			RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
diff --git a/src/backend/access/gin/gindatapage.c b/src/backend/access/gin/gindatapage.c
index 012225e..f322004 100644
--- a/src/backend/access/gin/gindatapage.c
+++ b/src/backend/access/gin/gindatapage.c
@@ -446,11 +446,21 @@ dataPlaceToPageLeaf(GinBtree btree, Buffer buf, GinBtreeStack *stack,
 	leafSegmentInfo *lastleftinfo;
 	ItemPointerData maxOldItem;
 	ItemPointerData remaining;
+	int			fillfactor;
 
 	Assert(GinPageIsData(page));
 
 	rbound = *GinDataPageGetRightBound(page);
 
+	/* Grab option values */
+	if (btree->index->rd_options)
+	{
+		GinOptions *options = (GinOptions *) btree->index->rd_options;
+		fillfactor = options->fillfactor;
+	}
+	else
+		fillfactor = GIN_DEFAULT_FILLFACTOR;
+
 	/*
 	 * Count how many of the new items belong to this page.
 	 */
@@ -511,15 +521,19 @@ dataPlaceToPageLeaf(GinBtree btree, Buffer buf, GinBtreeStack *stack,
 
 	/*
 	 * If we're appending to the end of the page, we will append as many items
-	 * as we can fit (after splitting), and stop when the pages becomes full.
-	 * Otherwise we have to limit the number of new items to insert, because
-	 * once we start packing we can't just stop when we run out of space,
-	 * because we must make sure that all the old items still fit.
+	 * as we can fit up to the given fillfactor at build (after splitting),
+	 * and stop when the pages becomes full at this rate. Otherwise we have to
+	 * limit the number of new items to insert, because once we start packing
+	 * we can't just stop when we run out of space, because we must make sure
+	 * that all the old items still

Re: [HACKERS] [PATCH] HINT: pg_hba.conf changed since last config reload

On Thu, Nov  6, 2014 at 05:46:42PM -0500, Peter Eisentraut wrote:
> Finally, the fact that a configuration change is in progress is
> privileged information.  Unprivileged users can deduct from the presence
> of this message that administrators are doing something, and possibly
> that they have done something wrong.
> 
> I think it's fine to log a message in the server log if the pg_hba.conf
> file needs reloading.  But the client shouldn't know about this at all.

Should we do this for postgresql.conf too?

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

  + Everyone has their own god. +


-- 
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] Add CREATE support to event triggers

On Wed, Nov 26, 2014 at 09:01:13PM -0500, Stephen Frost wrote:
> * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> > Bruce Momjian wrote:
> > > How would replicating DDL handle cases where the master and slave
> > > servers have different major versions and the DDL is only supported by
> > > the Postgres version on the master server?
> > 
> > Normally you would replicate between an older master and a newer
> > replica, so this shouldn't be an issue.  I find it unlikely that we
> > would de-support some syntax that works in an older version: it would
> > break pg_dump, for one thing.

I like the idea of older master/new replica, but what about
bidirectional replication?

Would the create/alter/drop WAL locical structure remain consistent
across major versions, or would the code have to read at least one older
version?  Would we limit it to one?

> While I tend to agree with you that it's not something we're likely to
> do, how would it break pg_dump?  We have plenty of version-specific
> logic in pg_dump and we could certainly generate a different syntax in
> a newer version than we did in an older version, if the newer server was
> expecting something different.  We've always held that you should use
> the version of pg_dump which match the server you are moving *to*, after
> all.

pg_upgrade avoids having to deal with major version changes by
leveraging pg_dump.  Is it possible to have the new replica use the new
pg_dump to connect to the old master to get a CREATE command that it can
execute?  Would that avoid having to ship CREATE syntax?  What it
wouldn't help with is ALTER and DROP though.  (We do have ALTER but I
think only for inheritance cases.)

> > In other words I view cross-version replication as a mechanism to
> > upgrade, not something that you would use permanently.  Once you
> > finish upgrading, promote the newer server and ditch the old master.
> 
> I agree with this also.

> > > If it would fail, does this limit the idea that logical replication
> > > allows major version-different replication?
> > 
> > Not in my view, at least.
> 
> I'm all for having logical replication be a way to do major version
> different replication (particularly for the purposes of upgrades), but
> it shouldn't mean we can never de-support a given syntax.
> 
> As one example, we've discussed a few times removing certain table-level
> privileges on the basis that they practically mean you own the table.
> Perhaps that'll never actually happen, but if it does, logical
> replication would need to deal with it.

Should we just tell the user not to modify the database schema during
this period?  Should we have a server mode which disables DDL?

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

  + Everyone has their own god. +


-- 
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] What exactly is our CRC algorithm?