Re: [HACKERS] [PATCH] server_version_num should be GUC_REPORT
On 9 January 2015 at 22:53, Tom Lane t...@sss.pgh.pa.us wrote: Craig Ringer cr...@2ndquadrant.com 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] Reducing buildfarm disk usage: remove temp installs when done
Andrew Dunstan and...@dunslane.net 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] pg_rewind in contrib
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 productnamePostgreSQL/ is started, it will start replay + from that checkpoint and apply all the required WAL.) + /para 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] Async execution of postgres_fdw.
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
horiguchi.kyot...@lab.ntt.co.jp 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 horiguchi.kyot...@lab.ntt.co.jp
Date: Tue, 13 Jan 2015 19:20:35 +0900
Subject: [PATCH] Asynchronous execution of postgres_fdw v5
This is the modified version of Asynchronous execution of
postgres_fdw.
- Do async fetch more aggressively than v3.
Re: [HACKERS] Patch: add recovery_timeout option to control timeout of restore_command nonzero status code
On Mon, Jan 19, 2015 at 4:10 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Sat, Jan 17, 2015 at 2:44 AM, Andres Freund and...@2ndquadrant.com 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: Partitioning: issues/ideas (Was: Re: [HACKERS] On partitioning)
On Fri, Jan 16, 2015 at 11:04 PM, Robert Haas robertmh...@gmail.com wrote: On Wed, Jan 14, 2015 at 9:07 PM, Amit Langote langote_amit...@lab.ntt.co.jp 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 through or binary-searchable. Also see the points about tuple-routing and
Re: [HACKERS] Reducing buildfarm disk usage: remove temp installs when done
On 01/18/2015 09:20 PM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net 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] proposal: disallow operator = and use it for named parameters
2015-01-19 4:54 GMT+01:00 Robert Haas robertmh...@gmail.com: On Sat, Jan 17, 2015 at 8:27 PM, Pavel Stehule pavel.steh...@gmail.com 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] Parallel Seq Scan
On Sat, Jan 17, 2015 at 10:09 AM, Robert Haas robertmh...@gmail.com wrote: On Fri, Jan 16, 2015 at 11:27 PM, Amit Kapila amit.kapil...@gmail.com 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] install libpq.dll in bin directory on Windows / Cygwin
On 1/15/15 2:37 AM, Michael Paquier wrote: On Wed, Dec 24, 2014 at 3:08 PM, Michael Paquier michael.paqu...@gmail.com 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)
On 19-01-2015 PM 12:37, Ashutosh Bapat wrote: On Fri, Jan 16, 2015 at 11:04 PM, Robert Haas robertmh...@gmail.com wrote: On Wed, Jan 14, 2015 at 9:07 PM, Amit Langote langote_amit...@lab.ntt.co.jp 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] pg_rewind in contrib
On Mon, Jan 19, 2015 at 2:38 PM, Michael Paquier michael.paqu...@gmail.com 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] TABLESAMPLE patch
On Sun, Jan 18, 2015 at 12:46 AM, Petr Jelinek p...@2ndquadrant.com 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 just wanted that we should check if the
implementation has any problem or not?
I think this is caused by random
Re: [HACKERS] hamerkop is stuck
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 n...@leadboat.com 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] New CF app deployment
On Tue, Jan 13, 2015 at 2:35 PM, Magnus Hagander mag...@hagander.net 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] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?
On Sun, Jan 18, 2015 at 4:09 PM, Tom Lane t...@sss.pgh.pa.us 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] Merging postgresql.conf and postgresql.auto.conf
On Sat, Jan 17, 2015 at 12:19 AM, Amit Kapila amit.kapil...@gmail.com 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] install libpq.dll in bin directory on Windows / Cygwin
On Mon, Jan 19, 2015 at 12:41 PM, Peter Eisentraut pete...@gmx.net 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: [HACKERS] pgaudit - an auditing extension for PostgreSQL
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 additional information regarding what kind
a command is codified in
Re: [HACKERS] proposal: disallow operator = and use it for named parameters
On Sat, Jan 17, 2015 at 8:27 PM, Pavel Stehule pavel.steh...@gmail.com 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] pgaudit - an auditing extension for PostgreSQL
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 dynamic shared memory block
to queue the logging messages to and then a background worker to write
them out
Re: [HACKERS] Patch: add recovery_timeout option to control timeout of restore_command nonzero status code
On Sat, Jan 17, 2015 at 2:44 AM, Andres Freund and...@2ndquadrant.com wrote:
On 2015-01-05 17:16:43 +0900, Michael Paquier wrote:
+ varlistentry id=restore-command-retry-interval
xreflabel=restore_command_retry_interval
+ termvarnamerestore_command_retry_interval/varname
(typeinteger/type)
+ indexterm
+primaryvarnamerestore_command_retry_interval/ recovery
parameter/primary
+ /indexterm
+ /term
+ listitem
+ para
+If varnamerestore_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
+literal5s/.
+ /para
+ para
+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.
+ /para
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 mich...@otacoo.com
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
/listitem
/varlistentry
+ varlistentry id=wal-availability-check-interval xreflabel=wal_availability_check_interval
+ termvarnamewal_availability_check_interval/varname (typeinteger/type)
+ indexterm
+primaryvarnamewal_availability_check_interval/ recovery parameter/primary
+ /indexterm
+ /term
+ listitem
+ para
+This parameter specifies the amount of time to wait when
+WAL is not available for a node in recovery. Default value is
+literal5s/.
+ /para
+ para
+A node in recovery will wait for this amount of time if
+varnamerestore_command/ returns
Re: [HACKERS] Reducing buildfarm disk usage: remove temp installs when done
Andrew Dunstan and...@dunslane.net 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] Reducing buildfarm disk usage: remove temp installs when done
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] moving from contrib to bin
On Sun, Jan 18, 2015 at 10:21 PM, Andres Freund and...@2ndquadrant.com wrote: On 2015-01-18 21:05:29 +0900, Michael Paquier wrote: On Sat, Jan 17, 2015 at 10:08 PM, Andres Freund and...@2ndquadrant.com 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] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?
On Sun, Jan 18, 2015 at 07:02:54PM -0500, Tom Lane wrote: Noah Misch n...@leadboat.com 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
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
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
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
[HACKERS] PGCon 2015
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
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
Re: [HACKERS] moving from contrib to bin
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
On Sat, Jan 17, 2015 at 10:08 PM, Andres Freund and...@2ndquadrant.com 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 mich...@otacoo.com 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 2clean /refsect1 refsect1 + titleAuthor/title + + para + Simon Riggs emailsi...@2ndquadrant.com/email + /para + /refsect1 + + refsect1 titleSee Also/title simplelist type=inline 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' /refsect1 refsect1 + titleAuthor/title + + para + Simon Riggs emailsi...@2ndquadrant.com/email + /para + /refsect1 + + refsect1 titleSee Also/title simplelist type=inline 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 /refsect1 refsect1 + titleAuthor/title + + para + Bruce Momjian emailbr...@momjian.us/email + /para + /refsect1 + + refsect1 titleSee Also/title simplelist type=inline diff --git
Re: [HACKERS] moving from contrib to bin
On Sun, Jan 18, 2015 at 9:56 PM, Andrew Gierth and...@tao11.riddles.org.uk 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] [REVIEW] Re: Fix xpath() to return namespace definitions
2015-01-18 10:44 GMT+07:00 Peter Eisentraut pete...@gmx.net: 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
Re: [HACKERS] moving from contrib to bin
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
On 2015-01-18 21:05:29 +0900, Michael Paquier wrote: On Sat, Jan 17, 2015 at 10:08 PM, Andres Freund and...@2ndquadrant.com 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
Simon, Bruce, Ants, (CCing..) On Sun, Jan 18, 2015 at 9:05 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Sat, Jan 17, 2015 at 10:08 PM, Andres Freund and...@2ndquadrant.com 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] proposal: row_to_array function
2015-01-17 7:26 GMT+01:00 Pavel Stehule pavel.steh...@gmail.com: 2015-01-16 22:35 GMT+01:00 Andrew Dunstan and...@dunslane.net: 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 http://BlueTreble.com 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
Re: [HACKERS] [REVIEW] Re: Fix xpath() to return namespace definitions
On Sun, Jan 18, 2015 at 06:05:05PM +0700, Ali Akbar wrote: 2015-01-18 10:44 GMT+07:00 Peter Eisentraut pete...@gmx.net: 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 da...@fetter.org 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
[HACKERS] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?
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] Reducing buildfarm disk usage: remove temp installs when done
On Mon, Jan 19, 2015 at 7:48 AM, Tom Lane t...@sss.pgh.pa.us 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
Re: [HACKERS] Re: Better way of dealing with pgstat wait timeout during buildfarm runs?
On Sun, Jan 18, 2015 at 04:09:29PM -0500, Tom Lane wrote: Noah Misch n...@leadboat.com 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] [REVIEW] Re: Fix xpath() to return namespace definitions
On Mon, Jan 19, 2015 at 2:38 AM, David Fetter da...@fetter.org wrote: On Sun, Jan 18, 2015 at 06:05:05PM +0700, Ali Akbar wrote: 2015-01-18 10:44 GMT+07:00 Peter Eisentraut pete...@gmx.net: 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 n...@leadboat.com 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 michael.paqu...@gmail.com writes: On Mon, Jan 19, 2015 at 2:38 AM, David Fetter da...@fetter.org wrote: On Sun, Jan 18, 2015 at 06:05:05PM +0700, Ali Akbar wrote: 2015-01-18 10:44 GMT+07:00 Peter Eisentraut pete...@gmx.net: 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
[HACKERS] Reducing buildfarm disk usage: remove temp installs when done
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?
Noah Misch n...@leadboat.com 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. */ ! backend_read_statsfile(); /*
