Re: [HACKERS] Parallel Seq Scan

[Amit Kapila]

On Sat, Jan 17, 2015 at 10:09 AM, Robert Haas  wrote:
> On Fri, Jan 16, 2015 at 11:27 PM, Amit Kapila 
wrote:
> > Assuming we will increment next_prefetch_block only after prefetching
> > blocks (equivalent to prefetch_increment), won't 2 workers can
> > simultaneously see the same value for next_prefetch_block and try to
> > perform prefetch for same blocks?
>
> The idea is that you can only examine and modify next_prefetch_block
> or next_scan_block while holding the mutex.
>
> > What will be value of prefetch_increment?
> > Will it be equal to prefetch_distance or prefetch_distance/2 or
> > prefetch_distance/4 or .. or will it be totally unrelated to
> > prefetch_distance?
>
> I dunno, that might take some experimentation.  prefetch_distance/2
> doesn't sound stupid.
>

Okay, I think I got the idea what you want to achieve via
prefetching.  So assuming prefetch_distance = 100 and
prefetch_increment = 50 (prefetch_distance /2), it seems to me
that as soon as there are less than 100 blocks in prefetch quota,
it will fetch next 50 blocks which means the system will be always
approximately 50 blocks ahead, that will ensure that in this algorithm
it will always perform sequential scan, however eventually this is turning
to be a system where one worker is reading from disk and then other
workers are reading from OS buffers to shared buffers and then getting
the tuple.  In this approach only one downside I can see and that is
there could be times during execution where some/all workers will have
to wait on the worker doing prefetching, however I think we should try
this approach and see how it works.

Another thing is that I think prefetching is not supported on all platforms
(Windows) and for such systems as per above algorithm we need to
rely on block-by-block method.


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

Re: [HACKERS] Patch: add recovery_timeout option to control timeout of restore_command nonzero status code

[Michael Paquier]

On Mon, Jan 19, 2015 at 4:10 PM, Michael Paquier
 wrote:
> On Sat, Jan 17, 2015 at 2:44 AM, Andres Freund  wrote:
>> Am I missing something or does this handle/affect streaming failures
>> just the same? That might not be a bad idea, because the current
>> interval is far too long for e.g. a sync replication setup. But it
>> certainly makes the name a complete misnomer.
> Hm.
Sorry for the typo here, I meant that I agree on that. But I am sure
you guessed it..
-- 
Michael


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

Re: [HACKERS] Patch: add recovery_timeout option to control timeout of restore_command nonzero status code

[Michael Paquier]

On Sat, Jan 17, 2015 at 2:44 AM, Andres Freund  wrote:
> On 2015-01-05 17:16:43 +0900, Michael Paquier wrote:
>> + > xreflabel="restore_command_retry_interval">
>> +  restore_command_retry_interval 
>> (integer)
>> +  
>> +restore_command_retry_interval recovery 
>> parameter
>> +  
>> +  
>> +  
>> +   
>> +If restore_command returns nonzero exit status code, 
>> retry
>> +command after the interval of time specified by this parameter,
>> +measured in milliseconds if no unit is specified. Default value is
>> +5s.
>> +   
>> +   
>> +This command is particularly useful for warm standby configurations
>> +to leverage the amount of retries done to restore a WAL segment file
>> +depending on the activity of a master node.
>> +   
>
> Don't really like this paragraph. What's "leveraging the amount of
> retries done" supposed to mean?
Actually I have reworked the whole paragraph with the renaming of the
parameter. Hopefully that's more clear.

>> +# restore_command_retry_interval
>> +#
>> +# specifies an optional retry interval for restore_command command, if
>> +# previous command returned nonzero exit status code. This can be useful
>> +# to increase or decrease the number of calls of restore_command.
> It's not really the number  but frequency, right?
Sure.

>> + else if (strcmp(item->name, "restore_command_retry_interval") 
>> == 0)
>> + {
>> + const char *hintmsg;
>> +
>> + if (!parse_int(item->value, 
>> &restore_command_retry_interval, GUC_UNIT_MS,
>> +&hintmsg))
>> + ereport(ERROR,
>> + 
>> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> +  errmsg("parameter \"%s\" 
>> requires a temporal value",
>> + 
>> "restore_command_retry_interval"),
>> +  hintmsg ? errhint("%s", 
>> _(hintmsg)) : 0));
>
> "temporal value" sounds odd to my ears. I'd rather keep in line with
> what guc.c outputs in such a case.
Yeah, I have been pondering about that a bit and I think that
wal_availability_check_interval is the closest thing as we want to
check if WAL is available for a walreceiver at record level and at
segment level for a restore_command.

> Am I missing something or does this handle/affect streaming failures
> just the same? That might not be a bad idea, because the current
> interval is far too long for e.g. a sync replication setup. But it
> certainly makes the name a complete misnomer.
Hm.

> Not this patch's fault, but I'm getting a bit tired seeing the above open
> coded. How about adding a function that does the sleeping based on a
> timestamptz and a ms interval?
You mean in plugins, right? I don't recall seeing similar patterns in
other code paths of backend. But I think that we can use something
like that in timestamp.c then because we need to leverage that between
two timestamps, the last failure and now():
TimestampSleepDifference(start_time, stop_time, internal_ms);
Perhaps you have something else in mind?

Attached is an updated patch.
-- 
Michael
From 0206c417f5b28eadb5f9d67ee0643320d7c0b621 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Mon, 19 Jan 2015 16:08:48 +0900
Subject: [PATCH] Add wal_availability_check_interval to control WAL fetching
 on failure

Default value is 5s.
---
 doc/src/sgml/recovery-config.sgml   | 21 +
 src/backend/access/transam/recovery.conf.sample | 10 +++
 src/backend/access/transam/xlog.c   | 39 -
 src/backend/utils/adt/timestamp.c   | 19 
 src/include/utils/timestamp.h   |  2 ++
 5 files changed, 83 insertions(+), 8 deletions(-)

diff --git a/doc/src/sgml/recovery-config.sgml b/doc/src/sgml/recovery-config.sgml
index 0c64ff2..7fd51ce 100644
--- a/doc/src/sgml/recovery-config.sgml
+++ b/doc/src/sgml/recovery-config.sgml
@@ -145,6 +145,27 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
   
  
 
+ 
+  wal_availability_check_interval (integer)
+  
+wal_availability_check_interval recovery parameter
+  
+  
+  
+   
+This parameter specifies the amount of time to wait when
+WAL is not available for a node in recovery. Default value is
+5s.
+   
+   
+A node in recovery will wait for this amount of time if
+restore_command returns nonzero exit status code when
+fetching new WAL segment files from archive or when a WAL receiver
+is not able to fetch a WAL record when using streaming replication.
+   
+  
+ 
+
 
 
   
diff --git a/src/backend/access/transam/recovery.conf.sample b/src/backend/access

Re: [HACKERS] proposal: disallow operator "=>" and use it for named parameters

[Pavel Stehule]

2015-01-19 4:54 GMT+01:00 Robert Haas :

> On Sat, Jan 17, 2015 at 8:27 PM, Pavel Stehule 
> wrote:
> > two years a operator "=>" is marked as deprecated (from PostgreSQL 9.2).
> >
> > Isn't time to use it for named parameters now (for PostgreSQL 9.5) ?
>
> I'm cool with that.  It's possible that there are installations out
> there that still have => operators installed, but every
> still-supported release warns you not to do that, and the hstore
> change exists in three released versions.  Anyway, no amount of
> waiting will eliminate the hazard completely.
>
> > I am sending a implementation where syntax based on "=>" symbol is second
> > (but preferred) variant of ":=" syntax .. syntax ":=" will be supported
> > still.
> >
> > Here is a patch
>
> I think you should just remove the WARNING, not change it to an error.
> If somebody wants to quote the operator name to be able to continue
> using it, I think that's OK.
>

I have no problem with it. Just I'll try if there are no some unexpected
problem and I'll send a updated patch

Regards

Pavel


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

Re: [HACKERS] pgaudit - an auditing extension for PostgreSQL

[Abhijit Menon-Sen]

Hello Stephen.

Thanks very much for having a look at the revised pgaudit.

At 2015-01-18 22:28:37 -0500, sfr...@snowman.net wrote:
>
> > 2. Set pgaudit.roles = 'r1, r2, …', which logs everything done by r1,
> >r2, and any of their descendants.
>
> If these roles were the 'audit' roles as considered under #1 then
> couldn't administrators control what pgaudit.roles provide today using
> role memberships granted to the roles listed?

Yes. But then there would be no convenient way to say "just log
everything this particular role does".

> > 3. Set pgaudit.log = 'read, write, …', which logs any events in any
> >of the listed classes.
> 
> Approach #1 addresses this too, doesn't it?

Approach #1 applies only to things you can grant permissions for. What
if you want to audit other commands?

> As currently implemented, role-level auditing is required to have DDL
> be logged, but you may very well want to log all DDL and no DML, for
> example.

Well, as formerly implemented, you could do this by adding r1 to .roles
and then setting .log appropriately. But you know that already and, if
I'm not mistaken, have said you don't like it.

I'm all for making it possible to configure fine-grained auditing, but
I don't think that should come at the expense of being able to whack
things with a simpler, heavier^Wmore inclusive hammer if you want to.

> My feeling is that we should strip down what goes into contrib to be
> 9.5 based.

I do not think I can express a constructive opinion about this. I will
go along with whatever people agree is best.

> I'm also wondering if pieces of that are now out-of-date with where
> core is.

Yes, they are. I'll clean that up.

> I don't particularly like how pgaudit will need to be kept in sync
> with what's in ProcessUtility (normal and slow).

Sure, it's a pain.

> I'd really like to see this additional information regarding what kind
> a command is codified in core.  Ideally, we'd have a single place
> which tracks the kind of command and possibly other information which
> can then be addressed all at once when new commands are added.

What would this look like, and is it a realistic goal for 9.5?

> Also, we could allow more granularity by using the actual classes
> which we already have for objects.

Explain?

> > /*
> >  * This module collects AuditEvents from various sources (event
> >  * triggers, and executor/utility hooks) and passes them to the
> >  * log_audit_event() function.
> >  *
> >  * An AuditEvent represents an operation that potentially affects a
> >  * single object. If an underlying command affects multiple objects,
> >  * multiple AuditEvents must be created to represent it.
> >  */
> 
> The above doesn't appear to be accurate (perhaps it once was?) as
> log_audit_event() only takes a single AuditEvent structure at a time
> and it's not a list.  I'm not sure that needs to change, just pointing
> out that the comment appears to be inaccurate.

If multiple AuditEvents are created (as the ExecutorCheckPerms_hook may
do), log_audit_event() will be called multiple times. The comment is
correct, but I'll try to make it more clear.

> I'm inclined to suggest that the decision be made earlier on about if
> a given action should be logged, as the earlier we do that the less
> work we have to do and we could avoid having to pass in things like
> 'granted' to the log_audit_event function at all.

I considered that, but it makes it much harder to review all of the
places where such decisions are made. It's partly because I wrote the
code in this way that I was able to quickly change it to work the way
you suggested (at least once I understood what you suggested).

I prefer the one-line function and a few «if (pgaudit_configured())»
checks (to avoid creating AuditEvents if they won't be needed at all)
and centralising the decision-making rather than spreading the latter
around the whole code.

> > /*
> >  * Takes a role OID and returns true if the role is mentioned in
> >  * pgaudit.roles or if it inherits from a role mentioned therein;
> >  * returns false otherwise.
> >  */
> > 
> > static bool
> > role_is_audited(Oid roleid)
> > {
> > …
> 
> The results here should be cached, as was discussed earlier, iirc.

I'll look into that, but it's a peripheral matter.

> Whatever is considered 'noise' should at least be explicitly listed,
> so we know we're covering everything.

OK.

> > /*
> >  * Takes an AuditEvent and, if it should_be_logged(), writes it to the
> >  * audit log. The AuditEvent is assumed to be completely filled in by
> >  * the caller (unknown values must be set to "" so that they can be
> >  * logged without error checking).
> >  */
> > 
> > static void
> > log_audit_event(AuditEvent *e)
> > {
> 
> So, clearly, this is an area which could use some improvement.
> Specifically, being able to write to an independent file instead of just
> using ereport(), supporting other output formats (JSON, maybe even a
> table..).  Also, have you considered using a dyna

Re: [HACKERS] Async execution of postgres_fdw.

[Kyotaro HORIGUCHI]

Hello, that's a silly mistake. fetch_seize = 1 in the v4
patch. This v5 patch is fixed at the point.

> But the v4 patch mysteriously accelerates this query, 6.5 seconds.
> 
> > =# EXPLAIN (ANALYZE ON, COSTS OFF) SELECT x.a, x.c, y.c
> >FROM ft1 AS x JOIN ft1 AS y on x.a = y.a;
...
> >  Execution time: 6512.043 ms

fetch_size was 1 at this run. I got about 13.0 seconds for
fetch_size = 100, which is about 19% faster than the original.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

===
15 17:18:49 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20150116.171849.109146500.horiguchi.kyot...@lab.ntt.co.jp>
> I revised the patch so that async scan will be done more
> aggressively, and took execution time for two very simple cases.
> 
> As the result, simple seq scan gained 5% and hash join of two
> foreign tables gained 150%. (2.4 times faster).
> 
> While measuring the performance, I noticed that each scan in a
> query runs at once rather than alternating with each other in
> many cases such as hash join or sorted joins and so. So I
> modified the patch so that async fetch is done more
> aggressively. The new v4 patch is attached. The following numbers
> are taken based on it.
> 
> 
> Simple seq scan for the first test.
> 
> > CREATE TABLE lt1 (a int, b timestamp, c text);
> > CREATE SERVER sv1 FOREIGN DATA WRAPPER postgres_fdw OPTIONS (host 
> > 'localhost');
> > CREATE USER MAPPING FOR PUBLIC SERVER sv1;
> > CREATE FOREIGN TABLE ft1 () SERVER sv1 OPTIONS (table_name 'lt1');
> > INSERT INTO lt1 (SELECT a, now(), repeat('x', 128) FROM generate_series(0, 
> > 99) a);
> 
> On this case, I took the the 10 times average of exec time of the
> following query for both master head and patched version.  The
> fetch size is 100.
> 
> > postgres=# EXPLAIN (ANALYZE ON, COSTS OFF) SELECT * FROM ft1;
> >  QUERY PLAN  
> > --
> >  Foreign Scan on ft1  (actual time=0.79 5..4175.706 rows=100 loops=1)
> >  Planning time: 0.060 ms
> >  Execution time: 4276.043 ms
> 
>   master head  : avg = 4256.621,  std dev = 17.099
>   patched pgfdw: avg = 4036.463,  std dev =  2.608
> 
> The patched version is faster by about 5%. This should be pure
> result of asynchronous fetching, not including the effect of
> early starting of remote execution in ExecInit.
> 
> Interestingly, as fetch_count gets larger, the gain raises in
> spite of the decrease of the number of query sending.
> 
>   master head  : avg = 2622.759,  std dev = 38.379
>   patched pgfdw: avg = 2277.622,  std dev = 27.269
> 
> About 15% gain. And for 1,
> 
>   master head  : avg = 2000.980,  std dev =  6.434
>   patched pgfdw: avg = 1616.793,  std dev = 13.192
> 
> 19%.. It is natural that exec time reduces along with increase of
> fetch size, but I haven't found the reason why the patch's gain
> also increases.
> 
> ==
> 
> The second case is a simple join of two foreign tables sharing
> one connection.
> 
> The master head runs this query in about 16 seconds with almost
> no fluctuation among multiple tries.
> 
> > =# EXPLAIN (ANALYZE ON, COSTS OFF) SELECT x.a, x.c, y.c
> >FROM ft1 AS x JOIN ft1 AS y on x.a = y.a;
> >   QUERY PLAN
> > 
> >  Hash Join (actual time=7541.831..15924.631 rows=100 loops=1)
> >Hash Cond: (x.a = y.a)
> >   ->  Foreign Scan on ft1 x (actual time=1.176..6553.480 rows=100 
> > loops=1)
> >   ->  Hash (actual time=7539.761..7539.761 rows=100 loops=1)
> >Buckets: 32768  Batches: 64  Memory Usage: 2829kB
> >->  Foreign Scan on ft1 y (actual time=1.067..6529.165 rows=100 
> > loops=1)
> >  Planning time: 0.223 ms
> >  Execution time: 15973.916 ms
> 
> But the v4 patch mysteriously accelerates this query, 6.5 seconds.
> 
> > =# EXPLAIN (ANALYZE ON, COSTS OFF) SELECT x.a, x.c, y.c
> >FROM ft1 AS x JOIN ft1 AS y on x.a = y.a;
> >QUERY PLAN
> > 
> >  Hash Join (actual time=2556.977..5812.937 rows=100 loops=1)
> >Hash Cond: (x.a = y.a)
> >   ->  Foreign Scan on ft1 x (actual time=32.689..1936.565 rows=100 
> > loops=1)
> >   ->  Hash (actual time=2523.810..2523.810 rows=100 loops=1)
> >Buckets: 32768  Batches: 64  Memory Usage: 2829kB
> >->  Foreign Scan on ft1 y (actual time=50.345..1928.411 rows=100 
> > loops=1)
> >  Planning time: 0.220 ms
> >  Execution time: 6512.043 ms
> 
> The result data seems not broken. I don't know the reason yet but
> I'll investigate it.
>From faea77944d4d3e3332d9723958f548356e3bceba Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Tue, 13 Jan 2015 19:20:35 +0900
Subject: [PATCH] Asynchronous execution of postgres_fdw v5

This is the modified ve

Re: [HACKERS] TABLESAMPLE patch

[Amit Kapila]

On Sun, Jan 18, 2015 at 12:46 AM, Petr Jelinek  wrote:

> On 17/01/15 13:46, Amit Kapila wrote:
>
>
>> 3.
>> @@ -332,6 +334,11 @@ set_rel_pathlist(PlannerInfo *root, RelOptInfo *rel,
>> /* Foreign table */
>> set_foreign_pathlist(root, rel, rte);
>> }
>> +else if (rte->tablesample != NULL)
>> +{
>> +/* Build sample scan on relation */
>> +set_tablesample_rel_pathlist(root, rel, rte);
>> +}
>>
>> Have you consider to have a different RTE for this?
>>
>>
> I assume you mean different RTEKind, yes I did, but the fact is that it's
> still a relation, just with different scan type and I didn't want to modify
> every piece of code which deals with RTE_RELATION to also deal with the new
> RTE for sample scan as it seems unnecessary. I am not strongly opinionated
> about this one though.
>
>
No issues, but it seems we should check other paths where
different handling could be required for tablesample scan.
In set_rel_size(), it uses normal path for heapscan (set_plain_rel_size())
for rel size estimates where it checks the presence of partial indexes
and that might effect the size estimates and that doesn't seem to
be required when we have to perform scan based on TableSample
method.
I think we can once check other places where some separate
handling for (rte->inh) is done to see if we need some different handing
for Tablesample scan.


>  4.
>> SampleNext()
>> a.
>> Isn't it better to code SampleNext() similar to SeqNext() and
>> IndexNext(), which encapsulate the logic to get the tuple in
>> another function(tbs_next() or something like that)?
>>
>
> Possibly, my thinking was that unlike the index_getnext() and
> heap_getnext(), this function would not be called from any other place so
> adding one more layer of abstraction didn't seem useful. And it would mean
> creating new ScanDesc struct, etc.
>
>
I think adding additional abstraction would simplify the function
and reduce the chance of discrepency, I think we need to almost
create a duplicate version of code for heapgetpage() method.
Yeah, I agree that we need to build structure like ScanDesc, but
still it will be worth to keep code consistent.



>
>> b.
>> Also don't we want to handle pruning of page while
>> scanning (heap_page_prune_opt()) and I observed
>> in heap scan API's after visibility check we do check
>> for serializable conflict (CheckForSerializableConflictOut()).
>>
>
> Both good points, will add.
>
>
Another one is PageIsAllVisible() check.


>  Another thing is don't we want to do anything for sync scans
>> for these method's as they are doing heap scan?
>>
>
> I don't follow this one tbh.
>
>
Refer parameter (HeapScanDesc->rs_syncscan) and syncscan.c.
Basically during sequiantial scan on same table by different
backends, we attempt to keep them synchronized to reduce the I/O.


>
>> c.
>> for bernoulli method, it will first get the tupoffset with
>> the help of function and then apply visibility check, it seems
>> that could reduce the sample set way lower than expected
>> in case lot of tuples are not visible, shouldn't it be done in reverse
>> way(first visibility check, then call function to see if we want to
>> include it in the sample)?
>> I think something similar is done in acquire_sample_rows which
>> seems right to me.
>>
>
> I don't think so, the way bernoulli works is that it returns every tuple
> with equal probability, so the visible tuples have same chance of being
> returned as the invisible ones so the issue should be smoothed away
> automatically (IMHO).
>
>
How will it get smoothen for cases when let us say
more than 50% of tuples are not visible.  I think its
due to Postgres non-overwritting storage architecture
that we maintain multiple version of rows in same heap,
otherwise for different kind of architecture (like mysql/oracle)
where multiple row versions are not maintained in same
heap, the same function (with same percentage) can return
entirely different number of rows.



> The acquire_sample_rows has limit on number of rows it returns so that's
> why it has to do the visibility check before as the problem you described
> applies there.
>
>
Even if in tablesample method, the argument value is in
percentage that doesn't mean not to give any consideration to
number of rows returned.  The only difference I could see is with
tablesample method the number of rows returned will not be accurate
number.  I think it is better to consider only visible rows.


> The reason for using the probability instead of tuple limit is the fact
> that there is no way to accurately guess number of tuples in the table at
> the beginning of scan. This method should actually be better at returning
> the correct number of tuples without dependence on how many of them are
> visible or not and how much space in blocks is used.
>
>
>
>> 5.
>>
>>
>> So above test yield 60% rows first time and 100% rows next time,
>> when the test has requested 80%.
>> I understand that sample percentage will result an approximate
>> number of rows, however I jus

Re: [HACKERS] pg_rewind in contrib

[Michael Paquier]

On Mon, Jan 19, 2015 at 2:38 PM, Michael Paquier
 wrote:
> Heikki Linnakangas wrote:
>> Addressed most of your comments, and Michael's. Another version attached.

Extra thing: src/bin/pg_rewind/Makefile surely forgets to clean up the
stuff from the regression tests:
+clean distclean maintainer-clean:
+   rm -f pg_rewind$(X) $(OBJS) xlogreader.c
-- 
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] pg_rewind in contrib

[Michael Paquier]

Heikki Linnakangas wrote:
> Addressed most of your comments, and Michael's. Another version attached.

Looking at the set of TAP tests, I think that those lines open again
the door of CVE-2014-0067 (vulnerability with make check) on Windows:
# Initialize master, data checksums are mandatory
remove_tree($test_master_datadir);
system_or_bail("initdb -N -A trust -D $test_master_datadir
>>$log_path");
IMO we should use standard_initdb in TestLib.pm instead as pg_regress
--config-auth would be used for SSPI. standard_initdb should be
extended a bit as well to be able to pass a path to logs with
/dev/null as default. TAP tests do not run on Windows, still I think
that it would be better to cover any eventuality in this area before
we forget. Already mentioned by Peter, but I think as well that the
new additions to TAP should be a separate patch.

Random thought (not related to this patch), have a new option in
initdb doing this legwork:
+   # Accept replication connections on master
+   append_to_file("$test_master_datadir/pg_hba.conf", qq(
+local replication all trust
+host replication all 127.0.0.1/32 trust
+host replication all ::1/128 trust
+));

I am still getting a warning when building with MSVC:
  xlogreader.obj : warning LNK4049: locally defined symbol
pg_crc32c_table imported
[C:\Users\ioltas\git\postgres\pg_rewind.vcxproj]
1 Warning(s)
0 Error(s)

Nitpicking: number of spaces here is incorrect:
+  that when PostgreSQL is started, it will start replay
+ from that checkpoint and apply all the required WAL.)
+ 

The header of src/bin/pg_rewind/Makefile mentions pg_basebackup:
+#-
+#
+# Makefile for src/bin/pg_basebackup

In this Makefile as well, I think that EXTRA_CLEAN can be removed:
 +EXTRA_CLEAN = $(RMGRDESCSOURCES) xlogreader.c
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] Reducing buildfarm disk usage: remove temp installs when done

[Tom Lane]

Andrew Dunstan  writes:
> On 01/18/2015 09:20 PM, Tom Lane wrote:
>> What I see on dromedary, which has been around a bit less than a year,
>> is that the at-rest space consumption for all 6 active branches is
>> 2.4G even though a single copy of the git repo is just over 400MB:
>> $ du -hsc pgmirror.git HEAD REL*
>> 416Mpgmirror.git
>> 363MHEAD
>> 345MREL9_0_STABLE
>> 351MREL9_1_STABLE
>> 354MREL9_2_STABLE
>> 358MREL9_3_STABLE
>> 274MREL9_4_STABLE
>> 2.4Gtotal

> This isn't happening for me. Here's crake:
> [andrew@emma root]$ du -shc pgmirror.git/ [RH]*/pgsql
> 218Mpgmirror.git/
> 149MHEAD/pgsql
> 134MREL9_0_STABLE/pgsql
> 138MREL9_1_STABLE/pgsql
> 140MREL9_2_STABLE/pgsql
> 143MREL9_3_STABLE/pgsql
> 146MREL9_4_STABLE/pgsql
> 1.1Gtotal

> Maybe you need some git garbage collection?

Weird ... for me, dromedary and prairiedog are both showing very similar
numbers.  Shouldn't GC be automatic?  These machines are not running
latest and greatest git (looks like 1.7.3.1 and 1.7.9.6 respectively),
maybe that has something to do with it?

A fresh clone from git://git.postgresql.org/git/postgresql.git right
now is 167MB (using dromedary's git version), so we're both showing
some bloat over the minimum possible repo size, but it's curious that
mine is so much worse.

But the larger point is that git fetch does not, AFAICT, have the same
kind of optimization that git clone does to do hard-linking when copying
an object from a local source repo.  With or without GC, the resulting
duplicative storage is going to be the dominant effect after awhile on a
machine tracking a full set of branches.

> An alternative would be to remove the pgsql directory at the end of the 
> run and thus do a complete fresh checkout each run. As you say it would 
> cost some time but save some space. At least it would be doable as an 
> option, not sure I'd want to make it non-optional.

What I was thinking is that a complete-fresh-checkout approach would
remove the need for the copy_source step that happens now, thus buying
back at least most of the I/O cost.  But that's only considering the
working tree.  The real issue here seems to be about having duplicative
git repos ... seems like we ought to be able to avoid that.

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] [PATCH] server_version_num should be GUC_REPORT

[Craig Ringer]

On 9 January 2015 at 22:53, Tom Lane  wrote:

> Craig Ringer  writes:
> > While looking into client code that relies on parsing server_version
> > instead of checking server_version_num, I was surprised to discover that
> > server_version_num isn't sent to the client by the server as part of the
> > standard set of parameters reported post-auth.
>
> Why should it be?  server_version is what's documented to be sent.
>

Because it makes sense to send it - it's obviously useful for clients, and
the same rationale that justifies adding server_version_num as a GUC
("parsing text versions is annoying, slow, and flakey") would also be a
good reason to send it to clients on first connection.

If the protocol were being designed now, frankly I think it'd make sense to
send *only* server_version_num and let the client query for the full text
version if it wants it. Bit late for that though.

Anyway, apply this and server_version_num *is* documented to be sent ;-)


>
> > The attached patch marks server_version_num GUC_REPORT and documents
> > that it's reported to the client automatically.
>
> I think this is just a waste of network bandwidth.  No client-side code
> could safely depend on its being available for many years yet, therefore
> they're going to keep using server_version.
>

By the same logic, isn't server_version_num its self useless, since no
client can rely on it being present so it'll just use server_version?

server_version_num was added in 8.2. It's now present in all even remotely
interesting PostgreSQL versions and can be relied upon to be present. Back
in 2006 you argued against its addition using much the same rationale you
are doing for this now:
http://www.postgresql.org/message-id/21507.1154223...@sss.pgh.pa.us . Time
flies; now it's useful and can be reasonably relied upon.

In the mean time a client can use server_version_num if sent, and fall back
to server_version otherwise. Just like you already have to do if querying
it via current_setting(...). This means that at least when connecting to
newer servers clients no longer have to do any stupid dances around parsing
"9.5beta1", "9.4.0mycustompatchedPg", etc.

Having this sent by GUC_REPORT would be very helpful for PgJDBC. It'd let
the driver avoid some round trips while being smarter about handling of
custom version strings.

The server's GUC_REPORT output and other startup content is 331 bytes of
payload according to Wireshark. Sending server_version_num will add, what,
26 bytes? That's not completely negligible, but compared to the *real*
costs paid in round-trips it's pretty tiny.

I expected this to be uncontroversial to the point where it'd just be
committed on the spot. As that is not the case I will add it to the next
commitfest.


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

Re: [HACKERS] install libpq.dll in bin directory on Windows / Cygwin

[Michael Paquier]

On Mon, Jan 19, 2015 at 12:41 PM, Peter Eisentraut  wrote:
> I have committed my mingw portion, but I cannot take on the MSVC part.
OK, no problem. Perhaps I should add a new entry in the next CF for
the MSVC portion?
-- 
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: Partitioning: issues/ideas (Was: Re: [HACKERS] On partitioning)

[Amit Langote]

On 19-01-2015 PM 12:37, Ashutosh Bapat wrote:
> On Fri, Jan 16, 2015 at 11:04 PM, Robert Haas  wrote:
> 
>> On Wed, Jan 14, 2015 at 9:07 PM, Amit Langote
>>  wrote:
>>
>>> I wonder if we could add a clause like DISTRIBUTED BY to complement
>>> PARTITION ON that represents a hash distributed/partitioned table (that
>>> could be a syntax to support sharded tables maybe; we would definitely
>>> want to move ahead in that direction I guess)
>>
>> Maybe eventually, but let's not complicate things by worrying too much
>> about that now.
>>
> 
> Instead we might want to specify which server (foreign or local) each of
> the partition go to, something like LOCATED ON clause for each of the
> partitions with default as local server.
> 

Given how things stand today, we do not allow DDL with the FDW
interface, unless I'm missing something. So, we are restricted to only
going the other way around, say,

CREATE FOREIGN TABLE partXX PARTITION OF parent SERVER ...;

assuming we like the proposed syntax -

CREATE TABLE child PARTITION OF parent;

I think this is also assuming we are relying on foreign table
inheritance. That is, both that partitioning is based on inheritance and
foreign tables support inheritance (which should be the case soon)

Still, I think Robert may be correct in that it would not be sooner that
we integrate foreign tables with partitioning scheme (I guess mostly the
syntax aspect of it).

Thanks,
Amit



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

Re: [HACKERS] Reducing buildfarm disk usage: remove temp installs when done

[Andrew Dunstan]

On 01/18/2015 09:20 PM, Tom Lane wrote:

Andrew Dunstan  writes:

On 01/18/2015 05:48 PM, Tom Lane wrote:

One of the biggest causes of buildfarm run failures is "out of disk
space".  That's not just because people are running buildfarm critters
on small slow machines; it's because "make check-world" is an enormous
space hog.  Some numbers from current HEAD:

I don't have an issue, but you should be aware that the buildfarm
doesn't in fact run "make check-world", and it doesn't to a test install
for each contrib module, since it runs "installcheck", not "check" for
those. It also cleans up some data directories as it goes.

Darn.  I knew that it didn't use check-world per se, but I'd supposed
it was doing something morally equivalent.  But I checked just now and
didn't see the space consumption of the pgsql.build + inst trees going
much above about 750MB, so it's clearly not as bad as "make check-world".

I think the patch I proposed is still worthwhile though, because it
looks like the buildfarm is doing this on a case-by-case basis and
missing some cases: I see the tmp_check directories for pg_upgrade and
test_decoding sticking around till the end of the run.  That could
be fixed in the script of course, but why not have pg_regress do it?

Also, investigating space consumption on my actual buildfarm critters,
it seems like there might be some low hanging fruit in terms of git
checkout management.  It looks to me like each branch has a git repo
that only shares objects that existed as of the initial cloning, so
that over time each branch eats more and more unshared space.  Also
I wonder about the value of keeping around a checked-out tree per
branch and copying it each time rather than just checking out fresh.
What I see on dromedary, which has been around a bit less than a year,
is that the at-rest space consumption for all 6 active branches is
2.4G even though a single copy of the git repo is just over 400MB:

$ du -hsc pgmirror.git HEAD REL*
416Mpgmirror.git
363MHEAD
345MREL9_0_STABLE
351MREL9_1_STABLE
354MREL9_2_STABLE
358MREL9_3_STABLE
274MREL9_4_STABLE
2.4Gtotal

It'd presumably be worse on a critter that's existed longer.

Curious to know if you've looked into alternatives here.  I realize
that the tradeoffs might be different with an external git repo,
but for one being managed by the buildfarm script, it seems like
we could do better than this space-wise, for (maybe) little time
penalty.  I'd be willing to do some experimenting if you don't have
time for it.



This isn't happening for me. Here's crake:

   [andrew@emma root]$ du -shc pgmirror.git/ [RH]*/pgsql
   218Mpgmirror.git/
   149MHEAD/pgsql
   134MREL9_0_STABLE/pgsql
   138MREL9_1_STABLE/pgsql
   140MREL9_2_STABLE/pgsql
   143MREL9_3_STABLE/pgsql
   146MREL9_4_STABLE/pgsql
   1.1Gtotal

Maybe you need some git garbage collection?

An alternative would be to remove the pgsql directory at the end of the 
run and thus do a complete fresh checkout each run. As you say it would 
cost some time but save some space. At least it would be doable as an 
option, not sure I'd want to make it non-optional.


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] Merging postgresql.conf and postgresql.auto.conf

[Robert Haas]

On Sat, Jan 17, 2015 at 12:19 AM, Amit Kapila  wrote:
> So are you telling that whenever we read, save the settings
> to some catalog (probably a new one)?

Which process are you imagining would do this?  Certainly not the postmaster.

Independently of that, it sounds like solving the problem from the
wrong end.  I think the idea of ALTER SYSTEM .. SET ought to be that
you should EITHER edit postgresql.conf for all your configuration
changes, OR you should use ALTER SYSTEM .. SET for all of your
changes.  If you mix-and-match, well, that's certainly allowed and it
will work and everything, but you - as a human being - might get
confused.  I am doubtful that any technological measures we take can
ever eliminate that confusion, because it doesn't result from some
defect of the system design, but rather from the fact that splitting
up your settings between multiple locations is inherently confusing,
especially if some settings are present in multiple locations with one
value overriding the others.

Imagine that you went out and bought a second wallet or purse.  Then
you take half of the stuff that is in your original one and put it in
the new one.  Then you order duplicates of 20% of the items and put
the copies in the wallet or purse that doesn't already contain a copy
of that item.  I predict that if you do this, you will sometimes get
confused about which one contains any particular item that you're
looking for.  But this is not anyone's fault except yours, and the
solution is simple: keep everything in one place.

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


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

Re: [HACKERS] proposal: disallow operator "=>" and use it for named parameters

[Robert Haas]

On Sat, Jan 17, 2015 at 8:27 PM, Pavel Stehule  wrote:
> two years a operator "=>" is marked as deprecated (from PostgreSQL 9.2).
>
> Isn't time to use it for named parameters now (for PostgreSQL 9.5) ?

I'm cool with that.  It's possible that there are installations out
there that still have => operators installed, but every
still-supported release warns you not to do that, and the hstore
change exists in three released versions.  Anyway, no amount of
waiting will eliminate the hazard completely.

> I am sending a implementation where syntax based on "=>" symbol is second
> (but preferred) variant of ":=" syntax .. syntax ":=" will be supported
> still.
>
> Here is a patch

I think you should just remove the WARNING, not change it to an error.
If somebody wants to quote the operator name to be able to continue
using it, I think that's OK.

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


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

Re: [HACKERS] install libpq.dll in bin directory on Windows / Cygwin

[Peter Eisentraut]

On 1/15/15 2:37 AM, Michael Paquier wrote:
> On Wed, Dec 24, 2014 at 3:08 PM, Michael Paquier
>  wrote:
>> Attached are two patches, one for MinGW/cygwin, a slightly modified
>> version from Peter and the second implementing the same thing but for
>> the MSVC scripts. The method for MSVC is similar to what is done in
>> Peter's patch: roughly it checks if SO_MAJOR_VERSION is present in the
>> Makefile of a given library, the path of Makefile is found by looking
>> at the location of the .rc in the vcproj file (could be better but I
>> could not come up with a better method). TBH, it would be good to be
>> completely consistent in the way we build things on Windows, and we
>> may as well consider a backpatch to fix this long-standing bug. The
>> MSVC patch removes of course the hack copying libpq.dll from lib/ to
>> bin/.
>>
>> I mentioned the fix for MSVC scripts as well here:
>> http://www.postgresql.org/message-id/cab7npqqiuepzphd3mmk+q7_cqqrkk_v1fvxknymri660z4d...@mail.gmail.com
> Peter, this patch is waiting for input for a couple of weeks. IMO, it
> would be good to finally get a fix for this bug, and we have patches
> for both MSVC (the patch I sent) and mingw (your stuff).

I have committed my mingw portion, but I cannot take on the MSVC part.




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

Re: Partitioning: issues/ideas (Was: Re: [HACKERS] On partitioning)

[Ashutosh Bapat]

On Fri, Jan 16, 2015 at 11:04 PM, Robert Haas  wrote:

> On Wed, Jan 14, 2015 at 9:07 PM, Amit Langote
>  wrote:
> > * It has been repeatedly pointed out that we may want to decouple
> > partitioning from inheritance because implementing partitioning as an
> > extension of inheritance mechanism means that we have to keep all the
> > existing semantics which might limit what we want to do with the special
> > case of it which is partitioning; in other words, we would find
> > ourselves in difficult position where we have to inject a special case
> > code into a very generalized mechanism that is inheritance.
> > Specifically, do we regard a partitions as pg_inherits children of its
> > partitioning parent?
>
> I don't think this is totally an all-or-nothing decision.  I think
> everyone is agreed that we need to not break things that work today --
> e.g. Merge Append.  What that implies for pg_inherits isn't altogether
> clear.
>
> > * Syntax: do we want to make it similar to one of the many other
> > databases out there? Or we could invent our own?
>
> Well, what I think we don't want is something that is *almost* like
> some other database but not quite.  I lean toward inventing our own
> since I'm not aware of something that we'd want to copy exactly.
>
> > I wonder if we could add a clause like DISTRIBUTED BY to complement
> > PARTITION ON that represents a hash distributed/partitioned table (that
> > could be a syntax to support sharded tables maybe; we would definitely
> > want to move ahead in that direction I guess)
>
> Maybe eventually, but let's not complicate things by worrying too much
> about that now.
>

Instead we might want to specify which server (foreign or local) each of
the partition go to, something like LOCATED ON clause for each of the
partitions with default as local server.


>
> > * Catalog: We would like to have a catalog structure suitable to
> > implement capabilities like multi-column partitioning, sub-partitioning
> > (with arbitrary number of levels in the hierarchy). I had suggested
> > that we create two new catalogs viz. pg_partitioned_rel,
> > pg_partition_def to store metadata about a partition key of a
> > partitioned relation and partition bound info of a partition,
> > respectively. Also, see the point about on-disk representation of
> > partition bounds
>
> I'm not convinced that there is any benefit in spreading this
> information across two tables neither of which exist today.  If the
> representation of the partitioning scheme is going to be a node tree,
> then there's no point in taking what would otherwise have been a List
> and storing each element of it in a separate tuple. The overarching
> point here is that the system catalog structure should be whatever is
> most convenient for the system internals; I'm not sure we understand
> what that is yet.
>
> > * It is desirable to treat partitions as pg_class relations with perhaps
> > a new relkind(s). We may want to choose an implementation where only the
> > lowest level relations in a partitioning hierarchy have storage; those
> > at the upper layers are mere placeholder relations though of course with
> > associated constraints determined by partitioning criteria (with
> > appropriate metadata entered into the additional catalogs).
>
> I think the storage-less parents need a new relkind precisely to
> denote that they have no storage; I am not convinced that there's any
> reason to change the relkind for the leaf nodes.  But that's been
> proposed, so evidently someone thinks there's a reason to do it.
>
> > I am not
> > quite sure if each kind of the relations involved in the partitioning
> > scheme have separate namespaces and, if they are, how we implement that
>
> I am in favor of having all of the nodes in the hierarchy have names
> just as relations do today -- pg_class.relname.  Anything else seems
> to me to be complex to implement and of very marginal benefit.  But
> again, it's been proposed.
>
> > * In the initial implementation, we could just live with partitioning on
> > a set of columns (and not arbitrary expressions of them)
>
> Seems quite fair.
>
> > * We perhaps do not need multi-column LIST partitions as they are not
> > very widely used and may complicate the implementation
>
> I agree that the use case is marginal; but I'm not sure it needs to
> complicate the implementation much.  Depending on how the
> implementation shakes out, prohibiting it might come to seem like more
> of a wart than allowing it.
>
> > * There are a number of suggestions about how we represent partition
> > bounds (on-disk) - pg_node_tree, RECORD (a composite type or the rowtype
> > associated with the relation itself), etc. Important point to consider
> > here may be that partition key may contain more than one column
>
> Yep.
>
> > * How we represent partition definition in memory (for a given
> > partitioned relation) - important point to remember is that such a
> > representation should be efficient to iterate t

Re: [HACKERS] pgaudit - an auditing extension for PostgreSQL

[Stephen Frost]

Abhijit,

Apologies, I've been meaning to go through this for quite some time.

* Abhijit Menon-Sen (a...@2ndquadrant.com) wrote:
> I've changed pgaudit to work as you suggested.

Great, glad to see that.

> A quick note on the implementation: pgaudit was already installing an
> ExecutorCheckPerms_hook anyway; I adapted code from ExecRTECheckPerms
> to check if the audit role has been granted any of the permissions
> required for the operation.

Sure, makes sense to me.

> This means there are three ways to configure auditing:
> 
> 1. GRANT … ON … TO audit, which logs any operations that correspond to
>the granted permissions.

I was thinking we would be able to configure which role this applies to,
rather than hard-coding 'audit' as *the* role.  Perhaps with a new GUC,
or the existing 'pgaudit.roles' GUC could be used.  Further, this can be
extrapolated out to cover auditing of roles by checking for role
membership in this role, see below.

> 2. Set pgaudit.roles = 'r1, r2, …', which logs everything done by r1,
>r2, and any of their descendants.

If these roles were the 'audit' roles as considered under #1 then
couldn't administrators control what pgaudit.roles provide today using
role memberships granted to the roles listed?

> 3. Set pgaudit.log = 'read, write, …', which logs any events in any of
>the listed classes.

Approach #1 addresses this too, doesn't it?  SELECT vs. UPDATE vs.
DELETE, etc?  I'm not sure that this really adds much as it isn't nearly
as granular as the GRANT-based approach since it applies to everything.

One of the really big and interesting distinctions to consider between
checking permissions granted to an 'audit' role vs. role membership is
how DDL is handled.  As currently implemented, role-level auditing is
required to have DDL be logged, but you may very well want to log all
DDL and no DML, for example.  What would be really helpful is to develop
a wiki to cover common use-cases and how to set up pgaudit for them.

> (This is a small change from the earlier behaviour where, if a role was
> listed in .roles, it was still subject to the .log setting. I find that
> more useful in practice, but since we're discussing Stephen's proposal,
> I implemented what he said.)

Right- the idea is to provide a very granular way of specifying what is
to be logged; much more than what the previous pair of GUCs provided.

> The new pgaudit.c is attached here for review. Nothing else has changed
> from the earlier submission; and everything is in the github repository
> (github.com/2ndQuadrant/pgaudit).

There's a few big questions we need to address with pgaudit to have it
go into our repo.

The first is what major version(s) we're targetting.  My feeling is that
we should strip down what goes into contrib to be 9.5 based.  I
certainly understand that you want to support earlier versions but I
don't think it makes sense to try and carry that baggage in contrib when
it won't ever be released with earlier versions.

The second is the USE_DEPARSE_FUNCTIONS-related bits.  I realize that
there's a goal to get additional things into 9.5 from that branch but it
doesn't make sense for the contrib pgaudit to include those components
prior to them actually being in core.  I'm also wondering if pieces of
that are now out-of-date with where core is.  If those parts are going
to go into 9.5 soon (which looks like it may happen..) then hopefully we
can just remove the #define's and clean up the code to use the new
capabilities.

Lastly, I'll echo a concern which Robert has raised which is- how do we
make sure that new commands, etc, are covered?  I don't particularly
like how pgaudit will need to be kept in sync with what's in
ProcessUtility (normal and slow).  I'm actually a bit hopeful that we
can work out a way for pgaudit to help with this through a regression
test which loops over all objects and tests logging each of them.

One of the important aspects of auditing is that what is requested to be
audited is and exactly that- no more, no less.  Reviewing the code makes
me pretty nervous about that actually happening properly, but that's
mostly due to the back-and-forth between different major versions and
the #ifdef/#ifndef's.

Additional comments in-line below.

> enum LogClass {
>   LOG_NONE = 0,
> 
>   /* SELECT */
>   LOG_READ = (1 << 0),
> 
>   /* INSERT, UPDATE, DELETE, TRUNCATE */
>   LOG_WRITE = (1 << 1),
> 
>   /* GRANT, REVOKE, ALTER … */
>   LOG_PRIVILEGE = (1 << 2),
> 
>   /* CREATE/DROP/ALTER ROLE */
>   LOG_USER = (1 << 3),
> 
>   /* DDL: CREATE/DROP/ALTER */
>   LOG_DEFINITION = (1 << 4),
> 
>   /* DDL: CREATE OPERATOR etc. */
>   LOG_CONFIG = (1 << 5),
> 
>   /* VACUUM, REINDEX, ANALYZE */
>   LOG_ADMIN = (1 << 6),
> 
>   /* Function execution */
>   LOG_FUNCTION = (1 << 7),
> 
>   /* Absolutely everything; not available via pgaudit.log */
>   LOG_ALL = ~(uint64)0
> };

I'd really like to see this ad

Re: [HACKERS] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?

[Robert Haas]

On Sun, Jan 18, 2015 at 4:09 PM, Tom Lane  wrote:
> After looking at the code, the minimum-change alternative would be more or
> less as attached: first, get rid of the long-obsolete proposition that
> autovacuum workers need fresher-than-usual stats; second, allow
> pgstat_vacuum_stat to accept stats that are moderately stale (the number
> given below allows them to be up to 50 seconds old); and third, suppress
> wait-timeout warnings when the call is from pgstat_vacuum_stat.  The third
> point is what we need to avoid unnecessary buildfarm failures.  The second
> point addresses the idea that we don't need to stress the stats collector
> too much for this.

I think this is too much of a good thing.  I don't see any reason why
autovacuum's statistics need to be fresher than normal, but I also
don't see any reason why they need to be less fresh.  I think
suppressing the warning is a good idea, but why only suppress it for
autovacuum?  How about just knocking the level down to, say, DEBUG1?

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


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

Re: [HACKERS] Reducing buildfarm disk usage: remove temp installs when done

[Tom Lane]

Andrew Dunstan  writes:
> On 01/18/2015 05:48 PM, Tom Lane wrote:
>> One of the biggest causes of buildfarm run failures is "out of disk
>> space".  That's not just because people are running buildfarm critters
>> on small slow machines; it's because "make check-world" is an enormous
>> space hog.  Some numbers from current HEAD:

> I don't have an issue, but you should be aware that the buildfarm 
> doesn't in fact run "make check-world", and it doesn't to a test install 
> for each contrib module, since it runs "installcheck", not "check" for 
> those. It also cleans up some data directories as it goes.

Darn.  I knew that it didn't use check-world per se, but I'd supposed
it was doing something morally equivalent.  But I checked just now and
didn't see the space consumption of the pgsql.build + inst trees going
much above about 750MB, so it's clearly not as bad as "make check-world".

I think the patch I proposed is still worthwhile though, because it
looks like the buildfarm is doing this on a case-by-case basis and
missing some cases: I see the tmp_check directories for pg_upgrade and
test_decoding sticking around till the end of the run.  That could
be fixed in the script of course, but why not have pg_regress do it?

Also, investigating space consumption on my actual buildfarm critters,
it seems like there might be some low hanging fruit in terms of git
checkout management.  It looks to me like each branch has a git repo
that only shares objects that existed as of the initial cloning, so
that over time each branch eats more and more unshared space.  Also
I wonder about the value of keeping around a checked-out tree per
branch and copying it each time rather than just checking out fresh.
What I see on dromedary, which has been around a bit less than a year,
is that the at-rest space consumption for all 6 active branches is
2.4G even though a single copy of the git repo is just over 400MB:

$ du -hsc pgmirror.git HEAD REL*
416Mpgmirror.git
363MHEAD
345MREL9_0_STABLE
351MREL9_1_STABLE
354MREL9_2_STABLE
358MREL9_3_STABLE
274MREL9_4_STABLE
2.4Gtotal

It'd presumably be worse on a critter that's existed longer.

Curious to know if you've looked into alternatives here.  I realize
that the tradeoffs might be different with an external git repo,
but for one being managed by the buildfarm script, it seems like
we could do better than this space-wise, for (maybe) little time
penalty.  I'd be willing to do some experimenting if you don't have
time for it.

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] New CF app deployment

[Michael Paquier]

On Tue, Jan 13, 2015 at 2:35 PM, Magnus Hagander  wrote:
> Further status updates to come as we start working on it...

Things are looking good so far. All the information has been synced up
between both apps for the current CF and the next one. One the switch
is done, I would recommend to each patch author and reviewer to check
the patches they are registered on, and do the necessary modifications
if we missed something. Error is human.

Note as well that the new CF app has a couple of differences with the
old app regarding the patch status:
- When a patch is marked as "returned with feedback", it is
automatically added to the next CF
- When a patch is marked as "rejected", it is *not* added in the next
CF, but you can still re-add a new entry manually in the next app.
So "rejected" means as well that a patch is marked as such because the
author does not have time/resources to work on it for the next CF(s).
Thanks,
-- 
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] hamerkop is stuck

[TAKATSUKA Haruka]

Hello.

Sorry about giving corrupted reports.
We examine what it is caused by now.

regards,

---
 TAKATSUKA Haruka  haru...@sraoss.co.jp
 SRA OSS, Inc.  http://www.sraoss.co.jp


On Fri, 16 Jan 2015 03:25:00 -0500
Noah Misch  wrote:

> Buildfarm member hamerkop stopped reporting in after commit f6dc6dd.  After
> commit 8d9cb0b, it resumed reporting in for 9.3 and earlier branches.  It is
> still silent for HEAD and REL9_4_STABLE.  It may have stale processes or stale
> lock files requiring manual cleanup.  Would you check it?
> 
> Thanks,
> nm
> 
> 
> -- 
> 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] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?

[Noah Misch]

On Sun, Jan 18, 2015 at 07:02:54PM -0500, Tom Lane wrote:
> Noah Misch  writes:
> > On Sun, Jan 18, 2015 at 04:09:29PM -0500, Tom Lane wrote:
> >> After looking at the code, the minimum-change alternative would be more or
> >> less as attached: first, get rid of the long-obsolete proposition that
> >> autovacuum workers need fresher-than-usual stats; second, allow
> >> pgstat_vacuum_stat to accept stats that are moderately stale (the number
> >> given below allows them to be up to 50 seconds old); and third, suppress
> >> wait-timeout warnings when the call is from pgstat_vacuum_stat.  The third
> >> point is what we need to avoid unnecessary buildfarm failures.  The second
> >> point addresses the idea that we don't need to stress the stats collector
> >> too much for this.
> 
> > Only #3 belongs in a minimum-change patch.  #1 and #2 solve and/or create
> > different problems and operate independently of #3.  A patch covering #3 
> > alone
> > sounds attractive.
> 
> [ raised eyebrow... ] In your previous message, you were advocating *for*
> a change that was more invasive than this one.  Why the change of heart?

I phrased that proposal based on a wrong belief that the collector writes the
stats file regularly in any case.  Vacuuming a 49s-old stats file without even
trying to get a fresh one might or might not improve PostgreSQL, but it's
dissimilar to what I had in mind.  I did not advocate for #1 at any point.

> In particular, I thought we'd already agreed to point #1.

Nope.  You and Alvaro ACKed it, then Heikki NACKed it.


-- 
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] moving from contrib to bin

[Michael Paquier]

On Sun, Jan 18, 2015 at 10:21 PM, Andres Freund  wrote:
> On 2015-01-18 21:05:29 +0900, Michael Paquier wrote:
>> On Sat, Jan 17, 2015 at 10:08 PM, Andres Freund  
>> wrote:
>> > Observations:
>> > 1) Are we sure it's a good idea to rely on pgxs.mk in src/bin programs?
>
>> Yeah, this seems like a bad dependency, PGXS being made for contrib
>> modules... So corrected in the patch attached (the headers of the
>> Makefiles are improved as well to be consistent with the other
>> utilities, btw there is code duplication in each Makefile if we do not
>> use PGXS stuff in src/bin).
>
> Yes, there's a fair amount of duplication. I do think there's a good
> case for making the Makefiles less redundant, but we should do that in
> all src/bin binaries, and in a separate patch.
Agreed on that. pg_ctl is a good candidate for improvement as well.

>> > 4) I have doubts that it's ok to integrate the tests in src/bin just the
>> >way they were done in contrib.
>> Are you referring to the tests of pg_upgrade?
> Yes.
I am not foreseeing any problems with the way they are done now as
long as they continue to use pg_regress to initialize the cluster.
Perhaps you have something on top of your mind?

>> > 5) Doing the msvc support for all intermediate commits in a separate
>> >commit strikes me as a bad idea. Essentially that makes the split
>> >pretty pointless.
>> > 6) Similarly I'd much rather see the doc movement in the same commit as
>> >the actually moved utility. Then we can start applying this one by one
>> >on whatever we have agreement.
>
>> Well, sure. The split was done just to facilitate review with stuff to
>> be applied directly on top of what Peter already did. And note that I
>> agree as well that everything should be done in a single commit.
>
> I don't think all of the moves should be done in one commit. I'd much
> rather do e.g. all the pg_upgrade stuff in one, and the rest in another.
OK. I am fine with that. pg_upgrade move touches backend code.

Now the remaining point seems to be: do we move pg_standby as well?
Seeing the last emails of this thread the answer would be to let it
end its life happily in contrib/.
-- 
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] Reducing buildfarm disk usage: remove temp installs when done

[Andrew Dunstan]

On 01/18/2015 05:48 PM, Tom Lane wrote:

One of the biggest causes of buildfarm run failures is "out of disk
space".  That's not just because people are running buildfarm critters
on small slow machines; it's because "make check-world" is an enormous
space hog.  Some numbers from current HEAD:

clean source tree:  120MB
built source tree:  400MB
tree after make check-world:3GB

(This is excluding ~250MB for one's git repo.)

The reason for all the bloat is the temporary install trees that we
create, which tend to eat up about 100MB apiece, and there are dozens
of them (eg, one per testable contrib module).  Those don't get removed
until the end of the test run, so the usage is cumulative.

The attached proposed patch removes each temp install tree as soon as
we're done with it, in the normal case where no error was detected.
This brings the peak space usage down from ~3GB to ~750MB.

To make things better in the buildfarm, we'd have to back-patch this into
all active branches, but I don't see any big problem with doing so.

Any objections?




I don't have an issue, but you should be aware that the buildfarm 
doesn't in fact run "make check-world", and it doesn't to a test install 
for each contrib module, since it runs "installcheck", not "check" for 
those. It also cleans up some data directories as it goes.


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: Fix xpath() to return namespace definitions

[Tom Lane]

Michael Paquier  writes:
> On Mon, Jan 19, 2015 at 2:38 AM, David Fetter  wrote:
>> On Sun, Jan 18, 2015 at 06:05:05PM +0700, Ali Akbar wrote:
>>> 2015-01-18 10:44 GMT+07:00 Peter Eisentraut :
 Btw, for bug-fix patches like this, should the patch creator (me) also
 create patches for back branches?

>> As I understand it, back-patches are the committer's responsibility.
>> The submitter might make suggestions as to how this might be
>> approached if it doesn't appear trivial.

> TBH, I would imagine that patches that can be applied to back-branches
> are a better start point than plain scratch particularly if there are
> diffs in stable branches compared to HEAD. Everybody's time is
> important.

Yeah --- and I'd argue that it's largely a waste of time to work on
creating back-branch patches until the HEAD patch is in final form.
Since we've generally reserved the right for the committer to whack
patches around before committing, I think this means the committer
also has to do the work of back-patch adjustment.

Now a committer who doesn't feel like doing that could certainly say
"here's the version of the HEAD patch that I'm willing to commit, but
it doesn't apply cleanly in back branches; could you work up back-branch
equivalents?".  But that hasn't been the usual approach.

regards, tom lane


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

Re: [HACKERS] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?

[Tom Lane]

Noah Misch  writes:
> On Sun, Jan 18, 2015 at 04:09:29PM -0500, Tom Lane wrote:
>> After looking at the code, the minimum-change alternative would be more or
>> less as attached: first, get rid of the long-obsolete proposition that
>> autovacuum workers need fresher-than-usual stats; second, allow
>> pgstat_vacuum_stat to accept stats that are moderately stale (the number
>> given below allows them to be up to 50 seconds old); and third, suppress
>> wait-timeout warnings when the call is from pgstat_vacuum_stat.  The third
>> point is what we need to avoid unnecessary buildfarm failures.  The second
>> point addresses the idea that we don't need to stress the stats collector
>> too much for this.

> Only #3 belongs in a minimum-change patch.  #1 and #2 solve and/or create
> different problems and operate independently of #3.  A patch covering #3 alone
> sounds attractive.

[ raised eyebrow... ] In your previous message, you were advocating *for*
a change that was more invasive than this one.  Why the change of heart?

In particular, I thought we'd already agreed to point #1.

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] [REVIEW] Re: Fix xpath() to return namespace definitions

[Michael Paquier]

On Mon, Jan 19, 2015 at 2:38 AM, David Fetter  wrote:
> On Sun, Jan 18, 2015 at 06:05:05PM +0700, Ali Akbar wrote:
>> 2015-01-18 10:44 GMT+07:00 Peter Eisentraut :
>> Btw, for bug-fix patches like this, should the patch creator (me) also
>> create patches for back branches?
>
> As I understand it, back-patches are the committer's responsibility.
> The submitter might make suggestions as to how this might be
> approached if it doesn't appear trivial.
TBH, I would imagine that patches that can be applied to back-branches
are a better start point than plain scratch particularly if there are
diffs in stable branches compared to HEAD. Everybody's time is
important.
--
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] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?

[Noah Misch]

On Sun, Jan 18, 2015 at 04:09:29PM -0500, Tom Lane wrote:
> Noah Misch  writes:
> > On Sat, Dec 27, 2014 at 07:55:05PM -0500, Tom Lane wrote:
> >> Or, much more simply, we could conclude that it's not that important
> >> if pgstat_vacuum_stat() is unable to get up-to-date stats, and rejigger
> >> the code so that we don't bleat when the file-reading request comes from
> >> that source.  This should be a back-patchable fix, whereas #2 above might
> >> be a bit too complicated for that.
> 
> > This, however, feels like a clear improvement.  When every VACUUM does
> > pgstat_vacuum_stat(), it doesn't matter if any given VACUUM misses removing
> > entries that became obsolete in the preceding second or so.  In fact, rather
> > than merely not bleating when the wait times out, let's not wait at all.  I
> > don't favor VACUUM of a small table taking 12s because it spent 2s on the
> > table and 10s waiting to garbage collect a piping-hot stats file.
> 
> I think that would be going too far: if an autovac wakes up in the dead of
> night, it should not use an ancient stats file merely because no one else
> has asked for stats recently.  But if it never waits at all, it'll be
> at the mercy of whatever is already on disk.

Whoops; I underestimated the upper bound on time between stats file writes.

> After looking at the code, the minimum-change alternative would be more or
> less as attached: first, get rid of the long-obsolete proposition that
> autovacuum workers need fresher-than-usual stats; second, allow
> pgstat_vacuum_stat to accept stats that are moderately stale (the number
> given below allows them to be up to 50 seconds old); and third, suppress
> wait-timeout warnings when the call is from pgstat_vacuum_stat.  The third
> point is what we need to avoid unnecessary buildfarm failures.  The second
> point addresses the idea that we don't need to stress the stats collector
> too much for this.

Only #3 belongs in a minimum-change patch.  #1 and #2 solve and/or create
different problems and operate independently of #3.  A patch covering #3 alone
sounds attractive.


-- 
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] Reducing buildfarm disk usage: remove temp installs when done

[Michael Paquier]

On Mon, Jan 19, 2015 at 7:48 AM, Tom Lane  wrote:
> To make things better in the buildfarm, we'd have to back-patch this into
> all active branches, but I don't see any big problem with doing so.
> Any objections?
Back-patching sounds like a good idea to me. At least this will allow
hamster to build all the active branches.
-- 
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] Reducing buildfarm disk usage: remove temp installs when done

[Tom Lane]

One of the biggest causes of buildfarm run failures is "out of disk
space".  That's not just because people are running buildfarm critters
on small slow machines; it's because "make check-world" is an enormous
space hog.  Some numbers from current HEAD:

clean source tree:  120MB
built source tree:  400MB
tree after make check-world:3GB

(This is excluding ~250MB for one's git repo.)

The reason for all the bloat is the temporary install trees that we
create, which tend to eat up about 100MB apiece, and there are dozens
of them (eg, one per testable contrib module).  Those don't get removed
until the end of the test run, so the usage is cumulative.

The attached proposed patch removes each temp install tree as soon as
we're done with it, in the normal case where no error was detected.
This brings the peak space usage down from ~3GB to ~750MB.

To make things better in the buildfarm, we'd have to back-patch this into
all active branches, but I don't see any big problem with doing so.

Any objections?

regards, tom lane

diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index caae3f0..ee3b80b 100644
*** a/src/test/regress/pg_regress.c
--- b/src/test/regress/pg_regress.c
*** regression_main(int argc, char *argv[], 
*** 2668,2673 
--- 2668,2686 
  		stop_postmaster();
  	}
  
+ 	/*
+ 	 * If there were no errors, remove the temp installation immediately to
+ 	 * conserve disk space.  (If there were errors, we leave the installation
+ 	 * in place for possible manual investigation.)
+ 	 */
+ 	if (temp_install && fail_count == 0 && fail_ignore_count == 0)
+ 	{
+ 		header(_("removing temporary installation"));
+ 		if (!rmtree(temp_install, true))
+ 			fprintf(stderr, _("\n%s: could not remove temp installation \"%s\": %s\n"),
+ 	progname, temp_install, strerror(errno));
+ 	}
+ 
  	fclose(logfile);
  
  	/*

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

Re: [HACKERS] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?

[Tom Lane]

Noah Misch  writes:
> On Sat, Dec 27, 2014 at 07:55:05PM -0500, Tom Lane wrote:
>> To get back to that original complaint about buildfarm runs failing,
>> I notice that essentially all of those failures are coming from "wait
>> timeout" warnings reported by manual VACUUM commands.  Now, VACUUM itself
>> has no need to read the stats files.  What's actually causing these
>> messages is failure to get a timely response in pgstat_vacuum_stat().
>> So let me propose a drastic solution: let's dike out this bit in vacuum.c:
>> 
>> /*
>> * Send info about dead objects to the statistics collector, unless we are
>> * in autovacuum --- autovacuum.c does this for itself.
>> */
>> if ((vacstmt->options & VACOPT_VACUUM) && !IsAutoVacuumWorkerProcess())
>> pgstat_vacuum_stat();
>> 
>> This would have the effect of transferring all responsibility for
>> dead-stats-entry cleanup to autovacuum.  For ordinary users, I think
>> that'd be just fine.  It might be less fine though for people who
>> disable autovacuum, if there still are any.

> Like Robert, I don't care for that.

I obviously failed to make it clear enough that that was meant as a straw
man, lets-think-outside-the-box proposal.

>> Or, much more simply, we could conclude that it's not that important
>> if pgstat_vacuum_stat() is unable to get up-to-date stats, and rejigger
>> the code so that we don't bleat when the file-reading request comes from
>> that source.  This should be a back-patchable fix, whereas #2 above might
>> be a bit too complicated for that.

> This, however, feels like a clear improvement.  When every VACUUM does
> pgstat_vacuum_stat(), it doesn't matter if any given VACUUM misses removing
> entries that became obsolete in the preceding second or so.  In fact, rather
> than merely not bleating when the wait times out, let's not wait at all.  I
> don't favor VACUUM of a small table taking 12s because it spent 2s on the
> table and 10s waiting to garbage collect a piping-hot stats file.

I think that would be going too far: if an autovac wakes up in the dead of
night, it should not use an ancient stats file merely because no one else
has asked for stats recently.  But if it never waits at all, it'll be
at the mercy of whatever is already on disk.

After looking at the code, the minimum-change alternative would be more or
less as attached: first, get rid of the long-obsolete proposition that
autovacuum workers need fresher-than-usual stats; second, allow
pgstat_vacuum_stat to accept stats that are moderately stale (the number
given below allows them to be up to 50 seconds old); and third, suppress
wait-timeout warnings when the call is from pgstat_vacuum_stat.  The third
point is what we need to avoid unnecessary buildfarm failures.  The second
point addresses the idea that we don't need to stress the stats collector
too much for this.

regards, tom lane

diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 0cf4988..b030e1c 100644
*** a/src/backend/postmaster/pgstat.c
--- b/src/backend/postmaster/pgstat.c
*** static void pgstat_write_statsfiles(bool
*** 259,265 
  static void pgstat_write_db_statsfile(PgStat_StatDBEntry *dbentry, bool permanent);
  static HTAB *pgstat_read_statsfiles(Oid onlydb, bool permanent, bool deep);
  static void pgstat_read_db_statsfile(Oid databaseid, HTAB *tabhash, HTAB *funchash, bool permanent);
! static void backend_read_statsfile(void);
  static void pgstat_read_current_status(void);
  
  static bool pgstat_write_statsfile_needed(void);
--- 259,265 
  static void pgstat_write_db_statsfile(PgStat_StatDBEntry *dbentry, bool permanent);
  static HTAB *pgstat_read_statsfiles(Oid onlydb, bool permanent, bool deep);
  static void pgstat_read_db_statsfile(Oid databaseid, HTAB *tabhash, HTAB *funchash, bool permanent);
! static void backend_read_statsfile(bool for_vacuum_stat);
  static void pgstat_read_current_status(void);
  
  static bool pgstat_write_statsfile_needed(void);
*** pgstat_vacuum_stat(void)
*** 951,959 
  
  	/*
  	 * If not done for this transaction, read the statistics collector stats
! 	 * file into some hash tables.
  	 */
! 	backend_read_statsfile();
  
  	/*
  	 * Read pg_database and make a list of OIDs of all existing databases
--- 951,962 
  
  	/*
  	 * If not done for this transaction, read the statistics collector stats
! 	 * file into some hash tables.  We are willing to accept stats that are
! 	 * more stale than usual here.  (Since VACUUM never runs in a transaction
! 	 * block, this cannot result in accepting stale stats that would be used
! 	 * for other purposes later in the transaction.)
  	 */
! 	backend_read_statsfile(true);
  
  	/*
  	 * Read pg_database and make a list of OIDs of all existing databases
*** pgstat_fetch_stat_dbentry(Oid dbid)
*** 2182,2188 
  	 * If not done for this transaction, read the statistics collector stats
  	 * file into some hash tables.
  	 

Re: [HACKERS] proposal: row_to_array function

[Pavel Stehule]

2015-01-17 7:26 GMT+01:00 Pavel Stehule :

>
> 2015-01-16 22:35 GMT+01:00 Andrew Dunstan :
>
>>
>> On 01/16/2015 12:22 PM, Pavel Stehule wrote:
>>
>>>
>>>
>>> There two possible transformations:
>>>
>>> row_to_array --> [[key1, value1],[key2, value2], ...]
>>> row_to_row_array --> [(key1, value1), (key2, value2), ... ]
>>>
>>>
>>> If we're going to go that route, I think it makes more sense to
>>> create an actual key/value type (ie:
>>> http://pgxn.org/dist/pair/doc/pair.html) and return an array of
>>> that.
>>>
>>>
>>> ok
>>>
>>> 
>>>
>>>
>>
>> I think we'd possibly be better off with simply returning a flat array,
>> [key1, value1, ...]
>>
>> Thats's what the hstore(text[]) and json_object(text[]) functions accept,
>> along with the 2D variant, if we want a precedent.
>>
>
> It can be one of supported variant. I should not be one, because we cannot
> to simply iterate over it
>
> Next possibility is teach FOREACH to take key and value in one step.
>

I looked to code and iteration over pair (key, value) is more simple

FOREACH supports target list, but source should be composite array.

ostgres=# do $$
declare a int;
  b int;
begin
  foreach a,b in array ARRAY[(1,2),(3,4)]
  loop
raise notice 'a = %, b = %', a,b;
  end loop;
end;
$$ language plpgsql;
NOTICE:  a = 1, b = 2
NOTICE:  a = 3, b = 4
DO

Conversion from ARRAY[k1,v1,k2,v2, ... ] is not well consistent with
current design


>
> Regards
>
> Pavel
>
>
>>
>> cheers
>>
>> andrew
>>
>>
>

[HACKERS] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?

[Noah Misch]

On Sat, Dec 27, 2014 at 07:55:05PM -0500, Tom Lane wrote:
> To get back to that original complaint about buildfarm runs failing,
> I notice that essentially all of those failures are coming from "wait
> timeout" warnings reported by manual VACUUM commands.  Now, VACUUM itself
> has no need to read the stats files.  What's actually causing these
> messages is failure to get a timely response in pgstat_vacuum_stat().
> So let me propose a drastic solution: let's dike out this bit in vacuum.c:
> 
> /*
>  * Send info about dead objects to the statistics collector, unless we are
>  * in autovacuum --- autovacuum.c does this for itself.
>  */
> if ((vacstmt->options & VACOPT_VACUUM) && !IsAutoVacuumWorkerProcess())
> pgstat_vacuum_stat();
> 
> This would have the effect of transferring all responsibility for
> dead-stats-entry cleanup to autovacuum.  For ordinary users, I think
> that'd be just fine.  It might be less fine though for people who
> disable autovacuum, if there still are any.

Like Robert, I don't care for that.

> Or, much more simply, we could conclude that it's not that important
> if pgstat_vacuum_stat() is unable to get up-to-date stats, and rejigger
> the code so that we don't bleat when the file-reading request comes from
> that source.  This should be a back-patchable fix, whereas #2 above might
> be a bit too complicated for that.

This, however, feels like a clear improvement.  When every VACUUM does
pgstat_vacuum_stat(), it doesn't matter if any given VACUUM misses removing
entries that became obsolete in the preceding second or so.  In fact, rather
than merely not bleating when the wait times out, let's not wait at all.  I
don't favor VACUUM of a small table taking 12s because it spent 2s on the
table and 10s waiting to garbage collect a piping-hot stats file.


-- 
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: Fix xpath() to return namespace definitions

[David Fetter]

On Sun, Jan 18, 2015 at 06:05:05PM +0700, Ali Akbar wrote:
> 2015-01-18 10:44 GMT+07:00 Peter Eisentraut :
> 
> > On 1/7/15 8:51 PM, Ali Akbar wrote:
> > > So now +1 for back-patching this.
> >
> > Done.  Was a bit of work to get it into all the old versions.
> >
> 
> Wow. Thanks.
> 
> Btw, for bug-fix patches like this, should the patch creator (me) also
> create patches for back branches?

As I understand it, back-patches are the committer's responsibility.

The submitter might make suggestions as to how this might be
approached if it doesn't appear trivial.

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

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


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

Re: [HACKERS] moving from contrib to bin

[Peter Eisentraut]

On 1/18/15 8:16 AM, Andres Freund wrote:
> On 2015-01-18 22:02:13 +0900, Michael Paquier wrote:
>> On Sun, Jan 18, 2015 at 9:56 PM, Andrew Gierth
>>> 2) in any event it is essentially deprecated in favour of standby_mode
>>> and therefore moving it to src/bin seems like a wrong move on two counts?
>> There were some discussions about that a couple of months ago, and the
>> conclusion was to not remove it. Please see here for more information:
>> http://www.postgresql.org/message-id/flat/545946e9.8060...@gmx.net#545946e9.8060...@gmx.net
> 
> Sure, but not removing doesn't imply moving it. The other tools don't
> have a replacement and are more or less actively maintained. I don't
> think that's really the case for pg_standby.  We do *not* want to give
> it more credence by declaring it to be part of core.

I don't really care much, but the discussion appeared to indicate that
pg_standby currently does not have a full replacement.




-- 
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] moving from contrib to bin

[Peter Eisentraut]

On 1/17/15 8:08 AM, Andres Freund wrote:
> 1) Are we sure it's a good idea to rely on pgxs.mk in src/bin programs?

I am.  Why not?



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

[HACKERS] PGCon 2015

[Dan Langille]

Is your PGCon 2015 submission going in today or tomorrow?

-- 
Dan Langille
http://langille.org/



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

Re: [HACKERS] moving from contrib to bin

[Alvaro Herrera]

Andres Freund wrote:
> Hi,
> 
> On 2015-01-18 12:56:59 +, Andrew Gierth wrote:

> > and therefore moving it to src/bin seems like a wrong move on two counts?
> 
> I personally agree that that pg_standby shouldn't be moved. Even if we
> could not find agreement to remove it, there's little reason to move it
> imo. My reading of the previous discussion was that we're far from alone
> with that position.

There was clear agreement on a set of things tomove, and as I recall
pg_standby was not in it.  Wasn't this patch series updated to comply
with what was agreed?

> That's why I'd like to have the patchseries in a way that individual
> moves can be applied separately.

Yeah.

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


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

Re: [HACKERS] moving from contrib to bin

[Andres Freund]

On 2015-01-18 21:05:29 +0900, Michael Paquier wrote:
> On Sat, Jan 17, 2015 at 10:08 PM, Andres Freund  
> wrote:
> > Observations:
> > 1) Are we sure it's a good idea to rely on pgxs.mk in src/bin programs?

> Yeah, this seems like a bad dependency, PGXS being made for contrib
> modules... So corrected in the patch attached (the headers of the
> Makefiles are improved as well to be consistent with the other
> utilities, btw there is code duplication in each Makefile if we do not
> use PGXS stuff in src/bin).

Yes, there's a fair amount of duplication. I do think there's a good
case for making the Makefiles less redundant, but we should do that in
all src/bin binaries, and in a separate patch.

> > 4) I have doubts that it's ok to integrate the tests in src/bin just the
> >way they were done in contrib.

> Are you referring to the tests of pg_upgrade?

Yes.

> > 5) Doing the msvc support for all intermediate commits in a separate
> >commit strikes me as a bad idea. Essentially that makes the split
> >pretty pointless.
> > 6) Similarly I'd much rather see the doc movement in the same commit as
> >the actually moved utility. Then we can start applying this one by one
> >on whatever we have agreement.

> Well, sure. The split was done just to facilitate review with stuff to
> be applied directly on top of what Peter already did. And note that I
> agree as well that everything should be done in a single commit.

I don't think all of the moves should be done in one commit. I'd much
rather do e.g. all the pg_upgrade stuff in one, and the rest in another.

> Separating things would break build on a platform or another if a
> build is done based on an intermediate state of this work, that would
> not be nice.

Yes, that's why commits in a series should be standalone. I.e. all
should work on all platforms. Possibly individual ones don't add
features, but they shouldn't break things.

> > 8) Why did you remove Peter as the git author?

> I applied on my local repo this series of patches after some bash-ing
> without preserving any meta data, so the author name has just been
> changed on the way, and then ran a simple git format to generate the
> whole set once again. Well, sorry if this was confusing, but let's be
> clear anyway: I have no intention to make mine the work of Peter (or
> any other people).

You know that you can apply patch series like Peter's (or from your tar
archive) using 'git am'? Which preserves the author, message and such?


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] moving from contrib to bin

[Andres Freund]

On 2015-01-18 22:02:13 +0900, Michael Paquier wrote:
> On Sun, Jan 18, 2015 at 9:56 PM, Andrew Gierth
> > 2) in any event it is essentially deprecated in favour of standby_mode
> > and therefore moving it to src/bin seems like a wrong move on two counts?
> There were some discussions about that a couple of months ago, and the
> conclusion was to not remove it. Please see here for more information:
> http://www.postgresql.org/message-id/flat/545946e9.8060...@gmx.net#545946e9.8060...@gmx.net

Sure, but not removing doesn't imply moving it. The other tools don't
have a replacement and are more or less actively maintained. I don't
think that's really the case for pg_standby.  We do *not* want to give
it more credence by declaring it to be part of core.

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] moving from contrib to bin

[Andres Freund]

Hi,

On 2015-01-18 12:56:59 +, Andrew Gierth wrote:
> Correct me if I'm wrong, but is it not the case that:

> 1) pg_standby was intended to be customizable code, even if usable as
> distributed, and

I don't think that really happened in reality though...

> 2) in any event it is essentially deprecated in favour of standby_mode

Right.

> and therefore moving it to src/bin seems like a wrong move on two counts?

I personally agree that that pg_standby shouldn't be moved. Even if we
could not find agreement to remove it, there's little reason to move it
imo. My reading of the previous discussion was that we're far from alone
with that position.  That's why I'd like to have the patchseries in a
way that individual moves can be applied separately.

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] moving from contrib to bin

[Michael Paquier]

On Sun, Jan 18, 2015 at 9:56 PM, Andrew Gierth
 wrote:
> Correct me if I'm wrong, but is it not the case that:
> 1) pg_standby was intended to be customizable code, even if usable as
> distributed, and
I am not sure about that.

> 2) in any event it is essentially deprecated in favour of standby_mode
> and therefore moving it to src/bin seems like a wrong move on two counts?
There were some discussions about that a couple of months ago, and the
conclusion was to not remove it. Please see here for more information:
http://www.postgresql.org/message-id/flat/545946e9.8060...@gmx.net#545946e9.8060...@gmx.net
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] moving from contrib to bin

[Andrew Gierth]

Correct me if I'm wrong, but is it not the case that:

1) pg_standby was intended to be customizable code, even if usable as
distributed, and

2) in any event it is essentially deprecated in favour of standby_mode

and therefore moving it to src/bin seems like a wrong move on two counts?

-- 
Andrew (irc:RhodiumToad)


-- 
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] moving from contrib to bin

[Michael Paquier]

Simon, Bruce, Ants,
(CCing..)

On Sun, Jan 18, 2015 at 9:05 PM, Michael Paquier
 wrote:
> On Sat, Jan 17, 2015 at 10:08 PM, Andres Freund  
> wrote:
>> 7) Are we sure that the authors in the affected contrib modules are ok
>>with their authorship notice being removed? I don't think Ants, Bruce
>>or Simon have a problem with that, but ...
> Yeah, agreed that I have really too aggressive with what I did. Let's
> CC all the authors on this thread and get directly their permission to
> process then. Some people may accept, other no, so let's see.

Just to give some background if you didn't follow closely this thread:
we are discussing about moving a couple of binary utilities from
contrib/ to src/bin/, utilities whose author is one of you. To make
documentation consistent with all the other bin/ utilities we are
thinking about removing the mention to the authors. Would you agree
with that? Here are the utilities impacted:
- pg_archivecleanup (Simon)
- pg_standby (Simon)
- pg_test_fsync (Bruce)
- pg_test_timing (Ants)
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] moving from contrib to bin

[Michael Paquier]

On Sat, Jan 17, 2015 at 10:08 PM, Andres Freund  wrote:
> Observations:
> 1) Are we sure it's a good idea to rely on pgxs.mk in src/bin programs?
Yeah, this seems like a bad dependency, PGXS being made for contrib
modules... So corrected in the patch attached (the headers of the
Makefiles are improved as well to be consistent with the other
utilities, btw there is code duplication in each Makefile if we do not
use PGXS stuff in src/bin).

> 4) I have doubts that it's ok to integrate the tests in src/bin just the
>way they were done in contrib.
Are you referring to the tests of pg_upgrade?

> 5) Doing the msvc support for all intermediate commits in a separate
>commit strikes me as a bad idea. Essentially that makes the split
>pretty pointless.
> 6) Similarly I'd much rather see the doc movement in the same commit as
>the actually moved utility. Then we can start applying this one by one
>on whatever we have agreement.
Well, sure. The split was done just to facilitate review with stuff to
be applied directly on top of what Peter already did. And note that I
agree as well that everything should be done in a single commit.
Separating things would break build on a platform or another if a
build is done based on an intermediate state of this work, that would
not be nice.

> 7) Are we sure that the authors in the affected contrib modules are ok
>with their authorship notice being removed? I don't think Ants, Bruce
>or Simon have a problem with that, but ...
Yeah, agreed that I have really too aggressive with what I did. Let's
CC all the authors on this thread and get directly their permission to
process then. Some people may accept, other no, so let's see.

> 8) Why did you remove Peter as the git author?
I applied on my local repo this series of patches after some bash-ing
without preserving any meta data, so the author name has just been
changed on the way, and then ran a simple git format to generate the
whole set once again. Well, sorry if this was confusing, but let's be
clear anyway: I have no intention to make mine the work of Peter (or
any other people).

> I've also pushed the git tree of these changes to
> http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=summary
> branch move-contrib-bins-to-bin
That's helpful. I just picked it up and built the patch attached that
can be applied on top of it, correcting the Makefiles and the
reference to the authors in the docs.

FWIW, my branch, based on yours is here:
https://github.com/michaelpq/postgres/tree/contrib_to_bin
Regards,
-- 
Michael
From 7b55ad274aa30d43cfdabb822f72257b3cec8336 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Sun, 18 Jan 2015 21:01:02 +0900
Subject: [PATCH] Fix Makefiles and re-add author references

---
 doc/src/sgml/ref/pgarchivecleanup.sgml |  8 
 doc/src/sgml/ref/pgstandby.sgml|  8 
 doc/src/sgml/ref/pgtestfsync.sgml  |  8 
 doc/src/sgml/ref/pgtesttiming.sgml |  8 
 src/bin/pg_archivecleanup/Makefile | 30 +++
 src/bin/pg_standby/Makefile| 30 +++
 src/bin/pg_test_fsync/Makefile | 28 ++---
 src/bin/pg_test_timing/Makefile| 28 ++---
 src/bin/pg_upgrade/Makefile| 37 ++
 src/bin/pg_xlogdump/Makefile   | 33 --
 src/bin/pgbench/Makefile   | 33 --
 11 files changed, 217 insertions(+), 34 deletions(-)

diff --git a/doc/src/sgml/ref/pgarchivecleanup.sgml b/doc/src/sgml/ref/pgarchivecleanup.sgml
index 2665c53..f9f32e9 100644
--- a/doc/src/sgml/ref/pgarchivecleanup.sgml
+++ b/doc/src/sgml/ref/pgarchivecleanup.sgml
@@ -197,6 +197,14 @@ archive_cleanup_command = 'pg_archivecleanup -d /mnt/standby/archive %r 2>>clean
  
 
  
+  Author
+
+  
+   Simon Riggs si...@2ndquadrant.com
+  
+ 
+
+ 
   See Also
 
   
diff --git a/doc/src/sgml/ref/pgstandby.sgml b/doc/src/sgml/ref/pgstandby.sgml
index 87d5043..698a6c2 100644
--- a/doc/src/sgml/ref/pgstandby.sgml
+++ b/doc/src/sgml/ref/pgstandby.sgml
@@ -380,6 +380,14 @@ recovery_end_command = 'del C:\pgsql.trigger.5442'
  
 
  
+  Author
+
+  
+   Simon Riggs si...@2ndquadrant.com
+  
+ 
+
+ 
   See Also
 
   
diff --git a/doc/src/sgml/ref/pgtestfsync.sgml b/doc/src/sgml/ref/pgtestfsync.sgml
index 3f76071..f704ab5 100644
--- a/doc/src/sgml/ref/pgtestfsync.sgml
+++ b/doc/src/sgml/ref/pgtestfsync.sgml
@@ -107,6 +107,14 @@ PostgreSQL documentation
  
 
  
+  Author
+
+  
+   Bruce Momjian br...@momjian.us
+  
+ 
+
+ 
   See Also
 
   
diff --git a/doc/src/sgml/ref/pgtesttiming.sgml b/doc/src/sgml/ref/pgtesttiming.sgml
index 9850538..d1279fc 100644
--- a/doc/src/sgml/ref/pgtesttiming.sgml
+++ b/doc/src/sgml/ref/pgtesttiming.sgml
@@ -294,6 +294,14 @@ Histogram of timing durations:
  
 
  
+  Author
+
+  
+   Ants Aasma ants.aa...@eesti.ee
+  
+ 
+
+ 
   See Also
 
   
diff --git

Re: [HACKERS] [REVIEW] Re: Fix xpath() to return namespace definitions

[Ali Akbar]

2015-01-18 10:44 GMT+07:00 Peter Eisentraut :

> On 1/7/15 8:51 PM, Ali Akbar wrote:
> > So now +1 for back-patching this.
>
> Done.  Was a bit of work to get it into all the old versions.
>

Wow. Thanks.

Btw, for bug-fix patches like this, should the patch creator (me) also
create patches for back branches?

Regards,
-- 
Ali Akbar