Re: [HACKERS] Clean switchover
On 2013-06-14 04:56:15 +0900, Fujii Masao wrote: > On Wed, Jun 12, 2013 at 9:48 PM, Stephen Frost wrote: > > * Magnus Hagander (mag...@hagander.net) wrote: > >> On Wed, Jun 12, 2013 at 1:48 PM, Andres Freund > >> wrote: > >> > On 2013-06-12 07:53:29 +0900, Fujii Masao wrote: > >> >> The attached patch fixes this problem. It just changes walsender so > >> >> that it > >> >> waits for all the outstanding WAL records to be replicated to the > >> >> standby > >> >> before closing the replication connection. > >> > > >> > Imo this is a fix that needs to get backpatched... The code tried to do > >> > this but failed, I don't think it really gives grounds for valid *new* > >> > concerns. > >> > >> +1 (without having looked at the code itself, it's definitely a > >> behaviour that needs to be fixed) > > > > Yea, I was also thinking it would be reasonable to backpatch this; it > > really looks like a bug that we're allowing this to happen today. > > > > So, +1 on a backpatch for me. > > +1. I think that we can backpatch to 9.1, 9.2 and 9.3. I marked the patch as ready for committer. > In 9.0, the standby doesn't send back any message to the master and > there is no way to know whether replication has been done up to > the specified location, so I don't think that we can backpatch. Agreed. 9.0 doesn't have enough infrastructure for this. > One note is, even if we backpatch, controlled switchover may require > the backup in order to follow the timeline switch, in 9.1 and 9.2. > If we want to avoid the backup in that case, we need to set up > the shared archive area between the master and the standby and > set recovery_target_timeline to 'latest'. Fixing this seems outside the scope of this patch... - and rather unlikely to be backpatchable. 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] [GENERAL] Floating point error
At 2013-03-06 10:04:25 +, laurenz.a...@wien.gv.at wrote: > > How about this elaboration? Sorry to nitpick, but I don't like that either, on the grounds that if I had been in Tom Duffey's place, this addition to the docs wouldn't help me to understand and resolve the problem. I'm not entirely convinced that any brief mention of extra_float_digits would suffice, but here's a proposed rewording: The setting controls the number of extra significant digits included when a floating point value is converted to text for output. With the default value of 0, the output is portable to every platform supported by PostgreSQL. Increasing it will produce output that is closer to the stored value, but may be unportable. It's a pretty generic sort of warning, but maybe it would help? (I'm marking this "Waiting on author" in the CF.) -- Abhijit -- 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] Reduce maximum error in tuples estimation after vacuum.
On Friday, June 14, 2013 2:05 PM Kyotaro HORIGUCHI wrote:
> Hello,
>
> Postgresql estimates the number of live tuples after the vacuum has
> left some buffers unscanned. This estimation does well for most cases,
> but makes completely different result with a strong imbalance of tuple
> density.
>
> For example,
>
> create table t (a int, b int);
> insert into t (select a, (random() * 10)::int from
> generate_series((select count(*) from t) + 1, 100) a); update t set
> b = b + 1 where a < (select count(*) from t) * 0.7; vacuum t; delete
> from t where a < (select count(*) from t) * 0.99;
>
> After this, pg_stat_user_tables.n_live_tup shows 417670 which is
> 41 times larger than the real number of rows 11.
Number should be 10001 not 11.
> And what makes it
> worse, autovacuum nor autoanalyze won't run until n_dead_tup goes above
> 8 times larger than the real number of tuples in the table for the
> default settings..
>
>
> | postgres=# select n_live_tup, n_dead_tup
> |from pg_stat_user_tables where relname='t'; n_live_tup |
> | n_dead_tup
> | +
> | 417670 | 0
> |
> | postgres=# select reltuples from pg_class where relname='t';
> | reltuples
> | ---
> | 417670
> |
> | postgres=# select count(*) from t;
> | count
> | ---
> | 10001
I have tried to reproduce the problem in different m/c's, but couldn't
reproduce it.
I have ran tests with default configuration.
Output on Windows:
---
postgres=# create table t (a int, b int);
CREATE TABLE
postgres=# insert into t (select a, (random() * 10)::int from
generate_serie
s((select count(*) from t) + 1, 100) a);
INSERT 0 100
postgres=# update t set b = b + 1 where a < (select count(*) from t) * 0.7;
UPDATE 69
postgres=# vacuum t;
VACUUM
postgres=# delete from t where a < (select count(*) from t) * 0.99;
DELETE 98
postgres=#
postgres=# select n_live_tup, n_dead_tup from pg_stat_user_tables where
relname=
't';
n_live_tup | n_dead_tup
+
10001 | 98
(1 row)
Output on Suse
postgres=# drop table if exists t;
create table t (a int, b int);
insert into t (select a, (random() * 10)::int from
generate_series((select count(*) from t) + 1, 100) a);
update t set b = b + 1 where a < (select count(*) from t) * 0.7;
vacuum t;
delete from t where a < (select count(*) from t) * 0.99;
vacuum t;
select c.relpages, s.n_live_tup, c.reltuples, (select count(*) from t) as
tuples, reltuples::float / (select count(*) from t) as ratio from
pg_stat_user_tables s, pg_class c where s.relname = 't' and c.relname =
't';DROP TABLE
postgres=# CREATE TABLE
postgres=# INSERT 0 100
postgres=# UPDATE 69
postgres=# VACUUM
postgres=# DELETE 98
postgres=# VACUUM
postgres=#
relpages | n_live_tup | reltuples | tuples | ratio
--++---++---
4425 | 10001 | 10001 | 10001 | 1
(1 row)
When I tried to run vactest.sh, it gives below error:
linux:~/akapila/vacuum_nlivetup> ./vactest.sh
./vactest.sh: line 11: syntax error near unexpected token `&'
./vactest.sh: line 11: `psql ${dbname} -c "vacuum verbose t" |&
egrep "INFO: *\"t\": found " | sed -e 's/^.* versions in \([0-9]*\)
.*$/\1/''
Can you help me in reproducing the problem by letting me know if I am doing
something wrong or results of test are not predictable?
With Regards,
Amit Kapila.
--
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] [PERFORM] In progress INSERT wrecks plans on table
(Cc: to pgsql-performance dropped, pgsql-hackers added.) At 2013-05-06 09:14:01 +0100, si...@2ndquadrant.com wrote: > > New version of patch attached which fixes a few bugs. I read the patch, but only skimmed the earlier discussion about it. In isolation, I can say that the patch applies cleanly and looks sensible for what it does (i.e., cache pgprocno to speed up repeated calls to TransactionIdIsInProgress(somexid)). In that sense, it's ready for committer, but I don't know if there's a better/more complete/etc. way to address the original problem. -- Abhijit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [9.4 CF 1] The Commitfest Slacker List
Folks, For 9.2, we adopted it as policy that anyone submitting a patch to a commitfest is expected to review at least one patch submitted by someone else. And that failure to do so would affect the attention your patches received in the future. For that reason, I'm publishing the list below of people who submitted patches and have not yet claimed any patch in the commitfest to review. For those of you who are contributing patches for your company, please let your boss know that reviewing is part of contributing, and that if you don't do the one you may not be able to do the other. The two lists below, idle submitters and committers, constitutes 26 people. This *outnumbers* the list of contributors who are busy reviewing patches -- some of them four or more patches. If each of these people took just *one* patch to review, it would almost entirely clear the list of patches which do not have a reviewer. If these folks continue not to do reviews, this commitfest will drag on for at least 2 weeks past its closure date. Andrew Geirth Nick White Peter Eisentrout Alexander Korotkov Etsuro Fujita Hari Babu Jameison Martin Jon Nelson Oleg Bartunov Chris Farmiloe Samrat Revagade Alexander Lakhin Mark Kirkwood Liming Hu Maciej Gajewski Josh Kuperschmidt Mark Wong Gurjeet Singh Robins Tharakan Tatsuo Ishii Karl O Pinc Additionally, the following committers are not listed as reviewers on any patch. Note that I have no way to search which ones might be *committers* on a patch, so these folks are not necessarily slackers (although with Bruce, we know for sure): Bruce Momjian (momjian) Michael Meskes (meskes) Teodor Sigaev (teodor) Andrew Dunstan (adunstan) Magnus Hagander (mha) Itagaki Takahiro (itagaki) -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [9.4 CF 1] Commitfest Week One
Current Stats: Needs Review: 61, Waiting on Author: 19, Ready for Committer: 11, Committed: 11, Returned with Feedback: 3, Rejected: 1 34 Patches are awaiting review, and do not have a reviewer assigned. In the last week, 3 patches have moved to Ready for Committer and 3 patches have been committed. 10 patches have been reviewed and are Waiting on Author, probably preparatory to moving to Returned with Feedback. At the current review/commit rate, this commitfest will not conclude until almost the end of July. Time to pick up the pace, people! For the first time, we have removed reviewers who did not post a review in 5 days. In the upcoming week, we will be moving patches which are Waiting on Author to Returned with Feedback without further discussion, if they have seen no activity for 5 or more days. This means that you can expect Review assignments on patches to be reasonably current. Thank you so SO much to Mike Blackwell for help with this! 26 people who submitted patches have not claimed any patch for review. More of this in an email of shame later. Among the patches needing review, several will need a lot of attention from committers, and should maybe be reviewed as soon as possible. Those include: * Extension Templates * minmax indexes * Auto-tuning checkpoint_segments And then there's the assortment of WAL/Hint Bit patches, which will no doubt have some toxic interactions: preserving forensic information, performance improvement by reducing WAL, XLogInsert scaling, preserving forensic information when we freeze, remove PD_ALL_VISIBLE. These shoud be addressed early so that we can work out the conflicts between them. So, some progress, but more is needed. --Josh Berkus CommitFest Manager -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE ... ALTER CONSTRAINT
At 2013-06-08 21:45:24 +0100, si...@2ndquadrant.com wrote: > > ALTER TABLE foo >ALTER CONSTRAINT fktable_fk_fkey DEFERRED INITIALLY IMMEDIATE; I read the patch (looks good), applied it on HEAD (fine), ran make check (fine), and played with it in a test database. It looks great, and from previous responses it's something a lot of people have wished for. I'm marking this ready for committer. -- Abhijit -- 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] Naming of ORDINALITY column
On 06/23/2013 08:00 PM, Andrew Gierth wrote: > OK, let's try to cover all the bases here in one go. > 1. Stick with "?column?" as a warning flag that you're not supposed to > be using this without aliasing it to something. How do I actually supply an alias which covers both columns? What does that look like, syntactically? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Naming of ORDINALITY column (was: Re: Review: UNNEST (and other functions) WITH ORDINALITY)
OK, let's try to cover all the bases here in one go. First, the spec definition of WITH ORDINALITY simply says that the column name in the result is not equivalent to any other identifier in the same (including the ). It is clear that the intention of the spec is that any non-positional reference to this column (which is defined as being positionally last) requires an alias at some point, whether directly attached to the or at an outer level. Second, all the documentation I've looked at for other databases that implement this feature (such as DB2, Teradata, etc.) takes it for granted that the user must always supply an alias, even though the syntax does not actually require one. None of the ones I've seen suggest that the ordinality column has a useful or consistent name if no alias is supplied. So, while clearly there's nothing stopping us from going beyond the spec and using a column name that people can refer to without needing an alias, it would be a significant divergence from common practice in other dbs. (iirc, it was my suggestion to David to use "?column?" in the first place for this reason.) So as I see it the options are: 1. Stick with "?column?" as a warning flag that you're not supposed to be using this without aliasing it to something. 2. Use some other fixed name like "ordinality" simply to allow people to do things like select ... from unnest(x) with ordinality; without having to bother to provide an alias, simply as a convenience, without regard for consistency with others. (This will result in a duplicate name if "x" is of a composite type containing a column called "ordinality", so the caller will have to provide an alias in that specific case or get an ambiguous reference error. Similarly if using some other SRF which defines its own return column names.) 3. Generate an actually unique name (probably pointless) 4. Something else I haven't thought of. My vote remains with option 1 here; I don't think users should be encouraged to assume that the ordinality column will have a known name. -- 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: FILTER for aggregates [was Re: [HACKERS] Department of Redundancy Department: makeNode(FuncCall) division]
David Fetter writes: > On Sun, Jun 23, 2013 at 07:44:26AM -0700, Kevin Grittner wrote: >> I think it is OK if that gets a syntax error. If that's the "worst >> case" I like this approach. > I think reducing the usefulness of error messages is something we need > to think extremely hard about before we do. Is there maybe a way to > keep the error messages even if by some magic we manage to unreserve > the words? Of the alternatives discussed so far, I don't really like anything better than adding another special case to base_yylex(). Robert opined in the other thread about RESPECT NULLS that the potential failure cases with that approach are harder to reason about, which is true, but that doesn't mean that we should accept failures we know of because there might be failures we don't know of. One thing that struck me while thinking about this is that it seems like we've been going about it wrong in base_yylex() in any case. For example, because we fold WITH followed by TIME into a special token WITH_TIME, we will fail on a statement beginning WITH time AS ... even though "time" is a legal ColId. But suppose that instead of merging the two tokens into one, we just changed the first token into something special; that is, base_yylex would return a special token WITH_FOLLOWED_BY_TIME and then TIME. We could then fix the above problem by allowing either WITH or WITH_FOLLOWED_BY_TIME as the leading keyword of a statement; and similarly for the few other places where WITH can be followed by an arbitrary identifier. Going on the same principle, we could probably let FILTER be an unreserved keyword while FILTER_FOLLOWED_BY_PAREN could be a type_func_name_keyword. (I've not tried this though.) This idea doesn't help much for OVER because one of the alternatives for over_clause is "OVER ColId", and I doubt we want to have base_yylex know all the alternatives for ColId. I also had no great success with the NULLS FIRST/LAST case: AFAICT the substitute token for NULLS still has to be fully reserved, meaning that something like "select nulls last" still doesn't work without quoting. We could maybe fix that with enough denormalization of the index_elem productions, but it'd be ugly. > Could well. I suspect we may need to rethink the whole way we do > grammar at some point, but that's for a later discussion when I (or > someone else) has something choate to say about it. It'd sure be interesting to know what the SQL committee's target parsing algorithm is. I find it hard to believe they're uninformed enough to not know that these random syntaxes they keep inventing are hard to deal with in LALR(1). Or maybe they really don't give a damn about breaking applications every time they invent a new reserved word? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Support for REINDEX CONCURRENTLY
On Sun, Jun 23, 2013 at 3:34 PM, Michael Paquier wrote: > OK. Please find an updated patch for the toast part. > > On Sat, Jun 22, 2013 at 10:48 PM, Andres Freund > wrote: >> On 2013-06-22 22:45:26 +0900, Michael Paquier wrote: >>> On Sat, Jun 22, 2013 at 10:34 PM, Andres Freund >>> wrote: >>> > On 2013-06-22 12:50:52 +0900, Michael Paquier wrote: >>> >> By looking at the comments of RelationGetIndexList:relcache.c, >>> >> actually the method of the patch is correct because in the event of a >>> >> shared cache invalidation, rd_indexvalid is set to 0 when the index >>> >> list is reset, so the index list would get recomputed even in the case >>> >> of shared mem invalidation. >>> > >>> > The problem I see is something else. Consider code like the following: >>> > >>> > RelationFetchIndexListIfInvalid(toastrel); >>> > foreach(lc, toastrel->rd_indexlist) >>> >toastidxs[i++] = index_open(lfirst_oid(lc), RowExclusiveLock); >>> > >>> > index_open calls relation_open calls LockRelationOid which does: >>> > if (res != LOCKACQUIRE_ALREADY_HELD) >>> >AcceptInvalidationMessages(); >>> > >>> > So, what might happen is that you open the first index, which accepts an >>> > invalidation message which in turn might delete the indexlist. Which >>> > means we would likely read invalid memory if there are two indexes. >>> And I imagine that you have the same problem even with >>> RelationGetIndexList, not only RelationGetIndexListIfInvalid, because >>> this would appear as long as you try to open more than 1 index with an >>> index list. >> >> No. RelationGetIndexList() returns a copy of the list for exactly that >> reason. The danger is not to see an outdated list - we should be >> protected by locks against that - but looking at uninitialized or reused >> memory. > OK, so I removed RelationGetIndexListIfInvalid (such things could be > an optimization for another patch) and replaced it by calls to > RelationGetIndexList to get a copy of rd_indexlist in a local list > variable, list free'd when it is not necessary anymore. > > It looks that there is nothing left for this patch, no? Compile error ;) gcc -O0 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -g -I../../../src/include -c -o index.o index.c index.c: In function 'index_constraint_create': index.c:1257: error: too many arguments to function 'index_update_stats' index.c: At top level: index.c:1785: error: conflicting types for 'index_update_stats' index.c:106: error: previous declaration of 'index_update_stats' was here index.c: In function 'index_update_stats': index.c:1881: error: 'FormData_pg_class' has no member named 'reltoastidxid' index.c:1883: error: 'FormData_pg_class' has no member named 'reltoastidxid' make[3]: *** [index.o] Error 1 make[2]: *** [catalog-recursive] Error 2 make[1]: *** [install-backend-recurse] Error 2 make: *** [install-src-recurse] Error 2 Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Support for REINDEX CONCURRENTLY
On Wed, Jun 19, 2013 at 9:50 AM, Michael Paquier wrote: > On Wed, Jun 19, 2013 at 12:36 AM, Fujii Masao wrote: >> On Tue, Jun 18, 2013 at 10:53 AM, Michael Paquier >> wrote: >>> An updated patch for the toast part is attached. >>> >>> On Tue, Jun 18, 2013 at 3:26 AM, Fujii Masao wrote: Here are the review comments of the removal_of_reltoastidxid patch. I've not completed the review yet, but I'd like to post the current comments before going to bed ;) *** a/src/backend/catalog/system_views.sql -pg_stat_get_blocks_fetched(X.oid) - -pg_stat_get_blocks_hit(X.oid) AS tidx_blks_read, -pg_stat_get_blocks_hit(X.oid) AS tidx_blks_hit +pg_stat_get_blocks_fetched(X.indrelid) - +pg_stat_get_blocks_hit(X.indrelid) AS tidx_blks_read, +pg_stat_get_blocks_hit(X.indrelid) AS tidx_blks_hit ISTM that X.indrelid indicates the TOAST table not the TOAST index. Shouldn't we use X.indexrelid instead of X.indrelid? >>> Indeed good catch! We need in this case the statistics on the index >>> and here I used the table OID. Btw, I also noticed that as multiple >>> indexes may be involved for a given toast relation, it makes sense to >>> actually calculate tidx_blks_read and tidx_blks_hit as the sum of all >>> stats of the indexes. >> >> Yep. You seem to need to change X.indexrelid to X.indrelid in GROUP clause. >> Otherwise, you may get two rows of the same table from pg_statio_all_tables. > I changed it a little bit in a different way in my latest patch by > adding a sum on all the indexes when getting tidx_blks stats. > doc/src/sgml/diskusage.sgml > There will be one index on the > TOAST table, if present. >> >> + table (see ). There will be one valid >> index >> + on the TOAST table, if present. There also might be indexes >> >> When I used gdb and tracked the code path of concurrent reindex patch, >> I found it's possible that more than one *valid* toast indexes appear. Those >> multiple valid toast indexes are viewable, for example, from pg_indexes. >> I'm not sure whether this is the bug of concurrent reindex patch. But >> if it's not, >> you seem to need to change the above description again. > Not sure about that. The latest code is made such as only one valid > index is present on the toast relation at the same time. > >> *** a/src/bin/pg_dump/pg_dump.c + "SELECT c.reltoastrelid, t.indexrelid " "FROM pg_catalog.pg_class c LEFT JOIN " - "pg_catalog.pg_class t ON (c.reltoastrelid = t.oid) " - "WHERE c.oid = '%u'::pg_catalog.oid;", + "pg_catalog.pg_index t ON (c.reltoastrelid = t.indrelid) " + "WHERE c.oid = '%u'::pg_catalog.oid AND t.indisvalid " + "LIMIT 1", Is there the case where TOAST table has more than one *valid* indexes? >>> I just rechecked the patch and is answer is no. The concurrent index >>> is set as valid inside the same transaction as swap. So only the >>> backend performing the swap will be able to see two valid toast >>> indexes at the same time. >> >> According to my quick gdb testing, this seems not to be true > Well, I have to disagree. I am not able to reproduce it. Which version > did you use? Here is what I get with the latest version of REINDEX > CONCURRENTLY patch... I checked with the following process: Sorry. This is my mistake. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: FILTER for aggregates [was Re: [HACKERS] Department of Redundancy Department: makeNode(FuncCall) division]
On Sun, Jun 23, 2013 at 07:44:26AM -0700, Kevin Grittner wrote: > Dean Rasheed wrote: > > > I'm still not happy that this patch is making FILTER a new reserved > > keyword, because I think it is a common enough English word (and an > > obscure enough SQL keyword) that people may well have used it for > > table names or aliases, and so their code will break. > > Well, if it is a useful capability with a standard syntax, I think > we should go with the standard syntax even if it might break > application code that uses, as unquoted identifiers, words reserved > by the SQL standard. Of course, non-reserved is better than > reserved if we can manage it. I'm not entirely sure that I agree with this. The SQL standard doesn't go adding keywords willy-nilly, or at least hasn't over a fairly long stretch of time. I can get precise numbers on this if needed. So far, only Tom and Greg have weighed in, Greg's response being here: http://www.postgresql.org/message-id/CAM-w4HOd3N_ozMygs==lm5+hu8yqkkayutgjinp6z2hazdr...@mail.gmail.com > > Playing around with the idea proposed there, it seems that it is > > possible to make FILTER (and also OVER) unreserved keywords, by > > splitting out the production of aggregate/window functions from normal > > functions in gram.y. Aggregate/window functions are not allowed in > > index_elem or func_table constructs, but they may appear in c_expr's. > > That resolves the shift/reduce conflicts that would otherwise occur > > from subsequent alias clauses, allowing FILTER and OVER to be > > unreserved. > > > > There is a drawback --- certain error messages become less specific, > > for example: "SELECT * FROM rank() OVER(ORDER BY random());" is now a > > syntax error, rather than the more useful error saying that window > > functions aren't allowed in the FROM clause. That doesn't seem like > > such a common case though. > > > > What do you think? > > I think it is OK if that gets a syntax error. If that's the "worst > case" I like this approach. I think reducing the usefulness of error messages is something we need to think extremely hard about before we do. Is there maybe a way to keep the error messages even if by some magic we manage to unreserve the words? > This also helps keep down the size of the generated parse tables, > doesn't it? Could well. I suspect we may need to rethink the whole way we do grammar at some point, but that's for a later discussion when I (or someone else) has something choate to say about it. Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] in-catalog Extension Scripts and Control parameters (templates?)
Jaime Casanova writes: > just tried to build this one, but it doesn't apply cleanly anymore... > specially the ColId_or_Sconst contruct in gram.y Will rebase tomorrow, thanks for the notice! -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- 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] in-catalog Extension Scripts and Control parameters (templates?)
On Wed, Apr 3, 2013 at 8:29 AM, Dimitri Fontaine wrote: > Hi, > > Dimitri Fontaine writes: >>> Documentation doesn't build, multiple errors. In addition to the reference >>> pages, there should be a section in the main docs about these templates. >> >> I'm now working on that, setting up the documentation tool set. > > Fixed in the attached version 6 of the patch. > just tried to build this one, but it doesn't apply cleanly anymore... specially the ColId_or_Sconst contruct in gram.y -- Jaime Casanova www.2ndQuadrant.com Professional PostgreSQL: Soporte 24x7 y capacitación Phone: +593 4 5107566 Cell: +593 987171157 -- 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] changeset generation v5-01 - Patches & git tree
On 2013-06-23 16:48:41 -0400, Tom Lane wrote: > Andres Freund writes: > > On 2013-06-23 08:27:32 -0700, Kevin Grittner wrote: > >> gcc: error: pg_receivellog.o: No such file or directory > >> make[3]: *** [pg_receivellog] Error 1 > > > I have seen that once as well. It's really rather strange since > > pg_receivellog.o is a clear prerequisite for pg_receivellog. I couldn't > > reproduce it reliably though, even after doing some dozen rebuilds or so. > > What versions of gmake are you guys using? It wouldn't be the first > time we've tripped over bugs in parallel make. See for instance > http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=1fc698cf14d17a3a8ad018cf9ec100198a339447 3.81 here. That was supposed to be the "safe" one, right? At least to the bugs seen/fixed recently. Kevin, any chance you still have more log than in the upthread mail available? 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] changeset generation v5-01 - Patches & git tree
Andres Freund writes: > On 2013-06-23 08:27:32 -0700, Kevin Grittner wrote: >> gcc: error: pg_receivellog.o: No such file or directory >> make[3]: *** [pg_receivellog] Error 1 > I have seen that once as well. It's really rather strange since > pg_receivellog.o is a clear prerequisite for pg_receivellog. I couldn't > reproduce it reliably though, even after doing some dozen rebuilds or so. What versions of gmake are you guys using? It wouldn't be the first time we've tripped over bugs in parallel make. See for instance http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=1fc698cf14d17a3a8ad018cf9ec100198a339447 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] changeset generation v5-01 - Patches & git tree
On 2013-06-23 10:32:05 -0700, Kevin Grittner wrote: > Andres Freund wrote: > > > Pushed and attached. > > The contrib/test_logical_decoding/sql/ddl.sql script is generating > unexpected results. For both table_with_pkey and > table_with_unique_not_null, updates of the primary key column are > showing: > > old-pkey: id[int4]:0 > > ... instead of the expected value of 2 or -2. > > See attached. Hm. Any chance this was an incomplete rebuild? I seem to remember having seen that once because some header dependency wasn't recognized correctly after applying some patch. Otherwise, could you give me: * the version you aplied the patch on * os/compiler Because I can't reproduce it, despite some playing around... 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] updated emacs configuration
Andrew Dunstan writes: > The idea is a very good one in principle, but my short experiment with > the provided .dir-locals.el didn't give me BSD style brace indentation. > It works if we can do those "unsafe" things, but then we surely don't > want to have a user prompted to accept the settings each time. If > .dir-locals.el won't do what we need on its own, it seems to me hardly > worth having. I'm un-thrilled with this as well, though for a slightly different reason: we have a policy that the PG sources should be tool agnostic, and in fact removed file-local emacs settings awhile back because of that. Even though I use emacs, I would much rather keep such settings in my personal library. (TBH, I keep enable-local-variables turned off, and would thus get no benefit from the proposed file anyhow.) Another thing I'm not too happy about is that the proposed patch summarily discards the information we had about how to work with older emacs versions. I'm not sure it will actually break anybody's setup, but that would depend on whether .dir-locals.el was added before or after the last round of rewriting of c-mode. 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] updated emacs configuration
On 06/23/2013 03:43 PM, Andrew Dunstan wrote: The idea is a very good one in principle, but my short experiment with the provided .dir-locals.el didn't give me BSD style brace indentation. It works if we can do those "unsafe" things, but then we surely don't want to have a user prompted to accept the settings each time. If .dir-locals.el won't do what we need on its own, it seems to me hardly worth having. However, it does work if you add this at the beginning of the c-mode list: (c-file-style . "bsd") 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] [PATCH] pgbench --throttle (submission 7 - with lag measurement)
An additional thought: Yet another thought, hopefully final on this subject. I think that the probability of a context switch is higher when calling PQfinish than in other parts of pgbench because it contains system calls (e.g. closing the network connection) where the kernel is likely to stop this process and activate another one. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated emacs configuration
On Thu, Jun 13, 2013 at 09:27:07PM -0400, Peter Eisentraut wrote: > I think the suggested emacs configuration snippets in > src/tools/editors/emacs.samples no longer represent current best > practices. I have come up with some newer things that I'd like to > propose for review. > ((c-mode . ((c-basic-offset . 4) > (fill-column . 79) I don't know whether you'd consider it to fall within the scope of this update, but 78 is the fill-column setting that matches pgindent. > (sgml-mode . ((fill-column . 79) >(indent-tabs-mode . nil SGML fill-column practice has varied widely. I estimate 72 is more the norm, though I personally favor longer lines like we use in source code comments. -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated emacs configuration
On 06/13/2013 09:27 PM, Peter Eisentraut wrote: I think the suggested emacs configuration snippets in src/tools/editors/emacs.samples no longer represent current best practices. I have come up with some newer things that I'd like to propose for review. First, I propose adding a .dir-locals.el file to the top-level directory with basic emacs settings. These get applied automatically. This especially covers the particular tab and indentation settings that PostgreSQL uses. With this, casual developers will not need to modify any of their emacs settings. (In the attachment, .dir-locals.el is called _dir-locals.el so that it doesn't get lost. To clarify, it goes into the same directory that contains configure.in.) With that, emacs.samples can be shrunk significantly. The only real reason to keep is that that c-offsets-alist and (more dubiously) sgml-basic-offset cannot be set from .dir-locals.el because they are not "safe". I have also removed many of the redundant examples and settled on a hook-based solution. I think together this setup would be significantly simpler and more practical. The idea is a very good one in principle, but my short experiment with the provided .dir-locals.el didn't give me BSD style brace indentation. It works if we can do those "unsafe" things, but then we surely don't want to have a user prompted to accept the settings each time. If .dir-locals.el won't do what we need on its own, it seems to me hardly worth having. 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] changeset generation v5-01 - Patches & git tree
On 2013-06-23 08:27:32 -0700, Kevin Grittner wrote: > Kevin Grittner wrote: > > > Confirmed that all 17 patch files now apply cleanly, and that `make > > check-world` builds cleanly after each patch in turn. > > Just to be paranoid, I did one last build with all 17 patch files > applied to 7dfd5cd21c0091e467b16b31a10e20bbedd0a836 using this > line: > > make maintainer-clean ; ./configure --prefix=$PWD/Debug --enable-debug > --enable-cassert --enable-depend --with-libxml --with-libxslt --with-openssl > --with-perl --with-python && make -j4 world > > and it died with this: > > gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith > -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute > -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g > -I../../../src/interfaces/libpq -I../../../src/include -D_GNU_SOURCE > -I/usr/include/libxml2 -c -o pg_receivexlog.o pg_receivexlog.c -MMD -MP -MF > .deps/pg_receivexlog.Po > gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith > -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute > -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g > -I. -I. -I../../../src/interfaces/libpq -I../../../src/bin/pg_dump > -I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o > mainloop.o mainloop.c -MMD -MP -MF .deps/mainloop.Po > gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith > -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute > -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g > pg_receivellog.o receivelog.o streamutil.o -L../../../src/port -lpgport > -L../../../src/common -lpgcommon -L../../../src/interfaces/libpq -lpq > -L../../../src/port -L../../../src/common -L/usr/lib -Wl,--as-needed > -Wl,-rpath,'/home/kgrittn/pg/master/Debug/lib',--enable-new-dtags -lpgport > -lpgcommon -lxslt -lxml2 -lssl -lcrypto -lz -lreadline -lcrypt -ldl -lm -o > pg_receivellog > gcc: error: pg_receivellog.o: No such file or directory > make[3]: *** [pg_receivellog] Error 1 > make[3]: Leaving directory `/home/kgrittn/pg/master/src/bin/pg_basebackup' > make[2]: *** [all-pg_basebackup-recurse] Error 2 > make[2]: *** Waiting for unfinished jobs I have seen that once as well. It's really rather strange since pg_receivellog.o is a clear prerequisite for pg_receivellog. I couldn't reproduce it reliably though, even after doing some dozen rebuilds or so. > It works with this patch-on-patch: > diff --git a/src/bin/pg_basebackup/Makefile b/src/bin/pg_basebackup/Makefile > index a41b73c..18d02f3 100644 > --- a/src/bin/pg_basebackup/Makefile > +++ b/src/bin/pg_basebackup/Makefile > @@ -42,6 +42,7 @@ installdirs: > uninstall: > rm -f '$(DESTDIR)$(bindir)/pg_basebackup$(X)' > rm -f '$(DESTDIR)$(bindir)/pg_receivexlog$(X)' > + rm -f '$(DESTDIR)$(bindir)/pg_receivellog$(X)' > > clean distclean maintainer-clean: > - rm -f pg_basebackup$(X) pg_receivexlog$(X) $(OBJS) pg_basebackup.o > pg_receivexlog.o pg_receivellog.o > + rm -f pg_basebackup$(X) pg_receivexlog$(X) pg_receivellog$(X) $(OBJS) > pg_basebackup.o pg_receivexlog.o pg_receivellog.o > > It appears to be an omission from file 0015. Yes, both are missing. > > + rm -f '$(DESTDIR)$(bindir)/pg_receivellog$(X)' > Oops. That part is not needed. Hm. Why not? I don't think either hunk has anything to do with that buildfailure though - can you reproduce the error without? Thanks, 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
[HACKERS] Re: backend hangs at immediate shutdown (Re: Back-branch update releases coming in a couple weeks)
On Sun, Jun 23, 2013 at 01:55:19PM +0900, MauMau wrote: > From: "Robert Haas" >> On Fri, Jun 21, 2013 at 10:02 PM, MauMau wrote: >>> I'm comfortable with 5 seconds. We are talking about the interval >>> between >>> sending SIGQUIT to the children and then sending SIGKILL to them. In >>> most >>> situations, the backends should terminate immediately. However, as I >>> said a >>> few months ago, ereport() call in quickdie() can deadlock >>> indefinitely. This >>> is a PostgreSQL's bug. >> >> So let's fix that bug. Then we don't need this. [quoted part filtered to undo caps lock] > There are two ways to fix that bug. You are suggesting 1 of the > following two methods, aren't you? I suspect Robert meant to prevent the deadlock by limiting quickdie() to calling only async-signal-safe functions. Implementing that looked grotty when we last discussed it, though, and it would not help against a library or trusted PL function suspending SIGQUIT. > 1. (Robert-san's idea) > Upon receipt of immediate shutdown request, postmaster sends SIGKILL to > its children. > > 2. (Tom-san's idea) > Upon receipt of immediate shutdown request, postmaster first sends > SIGQUIT to its children, wait a while for them to terminate, then sends > SIGKILL to them. > > > At first I proposed 1. Then Tom san suggested 2 so that the server is as > friendly to the clients as now by notifying them of the server shutdown. > I was convinced by that idea and chose 2. > > Basically, I think both ideas are right. They can solve the original > problem. Agreed. > However, re-considering the meaning of "immediate" shutdown, I feel 1 is > a bit better, because trying to do something in quickdie() or somewhere > does not match the idea of "immediate". We may not have to be friendly to Perhaps so, but more important than the definition of the word "immediate" is what an immediate shutdown has long meant in PostgreSQL. All this time it has made some effort to send a message to the client. Note also that the patch under discussion affects both immediate shutdowns and the automatic reset in response to a backend crash. I think the latter is the more-important use case, and the message is nice to have there. > the clients at the immediate shutdown. The code gets much simpler. In > addition, it may be better that we similarly send SIGKILL in backend > crash (FatalError) case, eliminate the use of SIGQUIT and remove > quickdie() and other SIGQUIT handlers. My take is that the client message has some value, and we shouldn't just discard it to simplify the code slightly. Finishing the shutdown quickly has value, of course. The relative importance of those things should guide the choice of a timeout under method #2, but I don't see a rigorous way to draw the line. I feel five seconds is, if anything, too high. What about using deadlock_timeout? It constitutes a rough barometer on the system's tolerance for failure detection latency, and we already overload it by having it guide log_lock_waits. The default of 1s makes sense to me for this purpose, too. We can always add a separate immediate_shutdown_timeout if there's demand, but I don't expect such demand. (If we did add such a GUC, setting it to zero could be the way to request method 1. If some folks strongly desire method 1, that's probably the way to offer it.) Thanks, nm -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- 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] changeset generation v5-01 - Patches & git tree
Andres Freund wrote: > Pushed and attached. The contrib/test_logical_decoding/sql/ddl.sql script is generating unexpected results. For both table_with_pkey and table_with_unique_not_null, updates of the primary key column are showing: old-pkey: id[int4]:0 ... instead of the expected value of 2 or -2. See attached. -- Kevin Grittner EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company regression.diffs Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BUG #7493: Postmaster messages unreadable in a Windows console
On Wed, Feb 20, 2013 at 04:00:00PM +0400, Alexander Law wrote:
> 15.02.2013 02:59, Noah Misch wrote:
With your proposed change, the problem will resurface in an actual
SQL_ASCII
database. At the problem's root is write_console()'s assumption that
messages
are in the database encoding. pg_bind_textdomain_codeset() tries to make
that
so, but it only works for encodings with a pg_enc2gettext_tbl entry. That
excludes SQL_ASCII, MULE_INTERNAL, and others. write_console() needs to
behave differently in such cases.
>>> Thank you for the notice. So it seems that "DatabaseEncoding" variable
>>> alone can't present a database encoding (for communication with a
>>> client) and current process messages encoding (for logging messages) at
>>> once. There should be another variable, something like
>>> CurrentProcessEncoding, that will be set to OS encoding at start and can
>>> be changed to encoding of a connected database (if
>>> bind_textdomain_codeset succeeded).
>> I'd call it MessageEncoding unless it corresponds with similar rigor to a
>> broader concept.
> Please look at the next version of the patch.
I studied this patch. I like the APIs it adds, but the implementation details
required attention.
Your patch assumes the message encoding will be a backend encoding, but this
need not be so; locale "Japanese (Japan)" uses CP932 aka SJIS. I've also
added the non-backend encodings to pg_enc2gettext_tbl; I'd welcome sanity
checks from folks who know those encodings well.
write_console() still garbled the payload in all cases where it used write()
to a console instead of WriteConsoleW(). You can observe this by configuring
Windows for Russian, running initdb with default locale settings, and running
"select 1/0" in template1. See comments for the technical details; I fixed
this by removing the logic to preemptively skip WriteConsoleW() for
encoding-related reasons. (Over in write_eventlog(), I am suspicious of the
benefits we get from avoiding ReportEventW() where possible. I have not
removed that, though.)
> --- a/src/backend/main/main.c
> +++ b/src/backend/main/main.c
> @@ -100,6 +100,8 @@ main(int argc, char *argv[])
>
> set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("postgres"));
>
> + SetMessageEncoding(GetPlatformEncoding());
SetMessageEncoding() and GetPlatformEncoding() can both elog()/ereport(), but
this is too early in the life of a PostgreSQL process for that.
The goal of this line of code is to capture gettext's default encoding, which
is platform-specific. On non-Windows platforms, it's the encoding implied by
LC_CTYPE. On Windows, it's the Windows ANSI code page. GetPlatformEncoding()
always gives the encoding implied by LC_CTYPE, which is therefore not correct
on Windows. You can observe broken output by setting your locale (in control
panel "Region and Language" -> Formats -> Format) to something unrelated to
your Windows ANSI code page (Region and Language -> Administrative -> Change
system locale...). Let's make PostgreSQL independent of gettext's
Windows-specific default by *always* calling bind_textdomain_codeset() on
Windows. In the postmaster and other non-database-attached processes, as well
as in SQL_ASCII databases, we would pass the encoding implied by LC_CTYPE.
Besides removing a discrepancy in PostgreSQL behavior between Windows and
non-Windows platforms, this has the great practical advantage of reducing
PostgreSQL's dependence on the deprecated Windows ANSI code page, which can
only be changed on a system-wide basis, by an administrator, after a reboot.
For message creation purposes, the encoding implied by LC_CTYPE on Windows is
somewhat arbitrary. Microsoft has deprecated them all in favor of UTF16, and
some locales (e.g. "Hindi (India)") have no legacy code page. I can see
adding a postmaster_encoding GUC to be used in place of consulting LC_CTYPE.
I haven't done that now; I just mention it for future reference.
On a !ENABLE_NLS build, messages are ASCII with database-encoding strings
sometimes interpolated. Therefore, such builds should keep the message
encoding equal to the database encoding.
The MessageEncoding was not maintained accurately on non-Windows systems. For
example, connecting to an lc_ctype=LATIN1, encoding=LATIN1 database does not
require a bind_textdomain_codeset() call on such systems, so we did not update
the message encoding. This was academic for the time being, because
MessageEncoding is only consulted on Windows.
The attached revision fixes all above points. Would you look it over? The
area was painfully light on comments, so I added some. I renamed
pgwin32_toUTF16(), which ceases to be a good name now that it converts from
message encoding, not database encoding. GetPlatformEncoding() became unused,
so I removed it. (If we have cause reintroduce the exact same concept later,
GetTTYEncoding() would name it more accurately.)
What should we do for the back branches, if
Re: [HACKERS] changeset generation v5-01 - Patches & git tree
Kevin Grittner wrote: > uninstall: > rm -f '$(DESTDIR)$(bindir)/pg_basebackup$(X)' > rm -f '$(DESTDIR)$(bindir)/pg_receivexlog$(X)' > + rm -f '$(DESTDIR)$(bindir)/pg_receivellog$(X)' Oops. That part is not needed. > clean distclean maintainer-clean: > - rm -f pg_basebackup$(X) pg_receivexlog$(X) $(OBJS) pg_basebackup.o > pg_receivexlog.o pg_receivellog.o > + rm -f pg_basebackup$(X) pg_receivexlog$(X) pg_receivellog$(X) $(OBJS) > pg_basebackup.o pg_receivexlog.o pg_receivellog.o Just that part. -- Kevin Grittner 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] changeset generation v5-01 - Patches & git tree
Kevin Grittner wrote: > Confirmed that all 17 patch files now apply cleanly, and that `make > check-world` builds cleanly after each patch in turn. Just to be paranoid, I did one last build with all 17 patch files applied to 7dfd5cd21c0091e467b16b31a10e20bbedd0a836 using this line: make maintainer-clean ; ./configure --prefix=$PWD/Debug --enable-debug --enable-cassert --enable-depend --with-libxml --with-libxslt --with-openssl --with-perl --with-python && make -j4 world and it died with this: gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -I../../../src/interfaces/libpq -I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o pg_receivexlog.o pg_receivexlog.c -MMD -MP -MF .deps/pg_receivexlog.Po gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -I. -I. -I../../../src/interfaces/libpq -I../../../src/bin/pg_dump -I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o mainloop.o mainloop.c -MMD -MP -MF .deps/mainloop.Po gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g pg_receivellog.o receivelog.o streamutil.o -L../../../src/port -lpgport -L../../../src/common -lpgcommon -L../../../src/interfaces/libpq -lpq -L../../../src/port -L../../../src/common -L/usr/lib -Wl,--as-needed -Wl,-rpath,'/home/kgrittn/pg/master/Debug/lib',--enable-new-dtags -lpgport -lpgcommon -lxslt -lxml2 -lssl -lcrypto -lz -lreadline -lcrypt -ldl -lm -o pg_receivellog gcc: error: pg_receivellog.o: No such file or directory make[3]: *** [pg_receivellog] Error 1 make[3]: Leaving directory `/home/kgrittn/pg/master/src/bin/pg_basebackup' make[2]: *** [all-pg_basebackup-recurse] Error 2 make[2]: *** Waiting for unfinished jobs It works with this patch-on-patch: diff --git a/src/bin/pg_basebackup/Makefile b/src/bin/pg_basebackup/Makefile index a41b73c..18d02f3 100644 --- a/src/bin/pg_basebackup/Makefile +++ b/src/bin/pg_basebackup/Makefile @@ -42,6 +42,7 @@ installdirs: uninstall: rm -f '$(DESTDIR)$(bindir)/pg_basebackup$(X)' rm -f '$(DESTDIR)$(bindir)/pg_receivexlog$(X)' + rm -f '$(DESTDIR)$(bindir)/pg_receivellog$(X)' clean distclean maintainer-clean: - rm -f pg_basebackup$(X) pg_receivexlog$(X) $(OBJS) pg_basebackup.o pg_receivexlog.o pg_receivellog.o + rm -f pg_basebackup$(X) pg_receivexlog$(X) pg_receivellog$(X) $(OBJS) pg_basebackup.o pg_receivexlog.o pg_receivellog.o It appears to be an omission from file 0015. -- Kevin Grittner 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] A better way than tweaking NTUP_PER_BUCKET
On 23.06.2013 01:48, Simon Riggs wrote: On 22 June 2013 21:40, Stephen Frost wrote: I'm actually not a huge fan of this as it's certainly not cheap to do. If it can be shown to be better than an improved heuristic then perhaps it would work but I'm not convinced. We need two heuristics, it would seem: * an initial heuristic to overestimate the number of buckets when we have sufficient memory to do so * a heuristic to determine whether it is cheaper to rebuild a dense hash table into a better one. Although I like Heikki's rebuild approach we can't do this every x2 overstretch. Given large underestimates exist we'll end up rehashing 5-12 times, which seems bad. It's not very expensive. The hash values of all tuples have already been calculated, so rebuilding just means moving the tuples to the right bins. Better to let the hash table build and then re-hash once, it we can see it will be useful. That sounds even less expensive, though. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: FILTER for aggregates [was Re: [HACKERS] Department of Redundancy Department: makeNode(FuncCall) division]
Dean Rasheed wrote: > I'm still not happy that this patch is making FILTER a new reserved > keyword, because I think it is a common enough English word (and an > obscure enough SQL keyword) that people may well have used it for > table names or aliases, and so their code will break. Well, if it is a useful capability with a standard syntax, I think we should go with the standard syntax even if it might break application code that uses, as unquoted identifiers, words reserved by the SQL standard. Of course, non-reserved is better than reserved if we can manage it. > Playing around with the idea proposed there, it seems that it is > possible to make FILTER (and also OVER) unreserved keywords, by > splitting out the production of aggregate/window functions from normal > functions in gram.y. Aggregate/window functions are not allowed in > index_elem or func_table constructs, but they may appear in c_expr's. > That resolves the shift/reduce conflicts that would otherwise occur > from subsequent alias clauses, allowing FILTER and OVER to be > unreserved. > > There is a drawback --- certain error messages become less specific, > for example: "SELECT * FROM rank() OVER(ORDER BY random());" is now a > syntax error, rather than the more useful error saying that window > functions aren't allowed in the FROM clause. That doesn't seem like > such a common case though. > > What do you think? I think it is OK if that gets a syntax error. If that's the "worst case" I like this approach. This also helps keep down the size of the generated parse tables, doesn't it? -- Kevin Grittner 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] changeset generation v5-01 - Patches & git tree
Andres Freund wrote: > The git tree is at: > git://git.postgresql.org/git/users/andresfreund/postgres.git branch > xlog-decoding-rebasing-cf4 > http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=shortlog;h=refs/heads/xlog-decoding-rebasing-cf4 > > On 2013-06-15 00:48:17 +0200, Andres Freund wrote: >> Overview of the attached patches: >> 0001: indirect toast tuples; required but submitted independently >> 0002: functions for testing; not required, >> 0003: (tablespace, filenode) syscache; required >> 0004: RelationMapFilenodeToOid: required, simple >> 0005: pg_relation_by_filenode() function; not required but useful >> 0006: Introduce InvalidCommandId: required, simple >> 0007: Adjust Satisfies* interface: required, mechanical, >> 0008: Allow walsender to attach to a database: required, needs review >> 0009: New GetOldestXmin() parameter; required, pretty boring >> 0010: Log xl_running_xact regularly in the bgwriter: required >> 0011: make fsync_fname() public; required, needs to be in a different file >> 0012: Relcache support for an Relation's primary key: required >> 0013: Actual changeset extraction; required >> 0014: Output plugin demo; not required (except for testing) but useful >> 0015: Add pg_receivellog program: not required but useful >> 0016: Add test_logical_decoding extension; not required, but contains >> the tests for the feature. Uses 0014 >> 0017: Snapshot building docs; not required > > Version v5-01 attached Confirmed that all 17 patch files now apply cleanly, and that `make check-world` builds cleanly after each patch in turn. Reviewing and testing the final result now. If that all looks good, will submit a separate review of each patch. Simon, do you want to do the final review and commit after I do each piece? -- Kevin Grittner 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] wrong state of patch in CF
Amit kapila wrote: > Patch (https://commitfest.postgresql.org/action/patch_view?id=1161) is already > committed by Commit > http://git.postgresql.org/pg/commitdiff/b23160889c963dfe23d8cf1f9be64fb3c535a2d6 > > It should be marked as Committed in CF. Done. Thanks! -- Kevin Grittner 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: Review [was Re: [HACKERS] MD5 aggregate]
On 21 June 2013 21:04, David Fetter wrote: > On Fri, Jun 21, 2013 at 10:48:35AM -0700, David Fetter wrote: >> On Mon, Jun 17, 2013 at 11:34:52AM +0100, Dean Rasheed wrote: >> > On 15 June 2013 10:22, Dean Rasheed wrote: >> > > There seem to be 2 separate directions that this could go, which >> > > really meet different requirements: >> > > >> > > 1). Produce an unordered sum for SQL to compare 2 tables regardless of >> > > the order in which they are scanned. A possible approach to this might >> > > be something like an aggregate >> > > >> > > md5_total(text/bytea) returns text >> > > >> > > that returns the sum of the md5 values of each input value, treating >> > > each md5 value as an unsigned 128-bit integer, and then producing the >> > > hexadecimal representation of the final sum. This should out-perform a >> > > solution based on numeric addition, and in typical cases, the result >> > > wouldn't be much longer than a regular md5 sum, and so would be easy >> > > to eyeball for differences. >> > > >> > >> > I've been playing around with the idea of an aggregate that computes >> > the sum of the md5 hashes of each of its inputs, which I've called >> > md5_total() for now, although I'm not particularly wedded to that >> > name. Comparing it with md5_agg() on a 100M row table (see attached >> > test script) produces interesting results: >> > >> > SELECT md5_agg(foo.*::text) >> > FROM (SELECT * FROM foo ORDER BY id) foo; >> > >> > 50bc42127fb9b028c9708248f835ed8f >> > >> > Time: 92960.021 ms >> > >> > SELECT md5_total(foo.*::text) FROM foo; >> > >> > 02faea7fafee4d253fc94cfae031afc43c03479c >> > >> > Time: 96190.343 ms >> > >> > Unlike md5_agg(), it is no longer a true MD5 sum (for one thing, its >> > result is longer) but it seems like it would be very useful for >> > quickly comparing data in SQL, since its value is not dependent on the >> > row-order making it easier to use and better performing if there is no >> > usable index for ordering. >> > >> > Note, however, that if there is an index that can be used for >> > ordering, the performance is not necessarily better than md5_agg(), as >> > this example shows. There is a small additional overhead per row for >> > initialising the MD5 sums, and adding the results to the total, but I >> > think the biggest factor is that md5_total() is processing more data. >> > The reason is that MD5 works on 64-byte blocks, so the total amount of >> > data going through the core MD5 algorithm is each row's size is >> > rounded up to a multiple of 64. In this simple case it ends up >> > processing around 1.5 times as much data: >> > >> > SELECT sum(length(foo.*::text)) AS md5_agg, >> >sum(((length(foo.*::text)+63)/64)*64) AS md5_total FROM foo; >> > >> > md5_agg | md5_total >> > +- >> > 8103815438 | 12799909248 >> > >> > although of course that overhead won't be as large on wider tables, >> > and even in this case the overall performance is still on a par with >> > md5_agg(). >> > >> > ISTM that both aggregates are potentially useful in different >> > situations. I would probably typically use md5_total() because of its >> > simplicity/order-independence and consistent performance, but >> > md5_agg() might also be useful when comparing with external data. >> > >> > Regards, >> > Dean > >> Performance review (skills needed: ability to time performance) >> >> Does the patch slow down simple tests? >> >> Yes. For an MD5 checksum of some random data, here are >> results from master: >> >> shackle@postgres:5493=# WITH t1 AS (SELECT >> string_agg(chr(floor(255*random()+1)::int),'') AS a FROM >> generate_series(1,1)), >> postgres-# t2 AS (SELECT a FROM t1 CROSS JOIN >> generate_series(1,1)) >> postgres-# select md5(a::text) FROM t2; >> shackle@postgres:5493=# \timing >> Timing is on. >> shackle@postgres:5493=# \g >> Time: 955.393 ms >> shackle@postgres:5493=# \g >> Time: 950.909 ms >> shackle@postgres:5493=# \g >> Time: 947.955 ms >> shackle@postgres:5493=# \g >> Time: 946.193 ms >> shackle@postgres:5493=# \g >> Time: 950.591 ms >> shackle@postgres:5493=# \g >> Time: 952.346 ms >> shackle@postgres:5493=# \g >> Time: 948.623 ms >> shackle@postgres:5493=# \g >> Time: 939.819 ms >> >> and here from master + the patch: >> >> WITH t1 AS (SELECT >> string_agg(chr(floor(255*random()+1)::int),'') AS a FROM >> generate_series(1,1)), >> t2 AS (SELECT a FROM t1 CROSS JOIN generate_series(1,1)) >> select md5(a::text) FROM t2; >> Time: 1129.178 ms >> shackle@postgres:5494=# \g >> Time: 1134.013 ms >> shackle@postgres:5494=# \g >> Time: 1124.387 ms >>
Re: FILTER for aggregates [was Re: [HACKERS] Department of Redundancy Department: makeNode(FuncCall) division]
On 21 June 2013 10:02, Dean Rasheed wrote: > On 21 June 2013 06:16, David Fetter wrote: >> On Fri, Jun 21, 2013 at 12:10:25AM -0400, Alvaro Herrera wrote: >>> David Fetter escribió: >>> > On Thu, Jun 20, 2013 at 08:59:27PM +0100, Dean Rasheed wrote: >>> >>> > > In my testing of sub-queries in the FILTER clause (an extension to the >>> > > spec), I was able to produce the following error: >>> > >>> > Per the spec, >>> > >>> > B) A shall not contain a , a >> > function>, or an outer reference. >>> >>> If this is not allowed, I think there should be a clearer error message. >>> What Dean showed is an internal (not user-facing) error message. >> >> Please find attached a patch which allows subqueries in the FILTER >> clause and adds regression testing for same. >> > > Nice! > > I should have pointed out that it was already working for some > sub-queries, just not all. It's good to see that it was basically just > a one-line fix, because I think it would have been a shame to not > support sub-queries. > > I hope to take another look at it over the weekend. > I'm still not happy that this patch is making FILTER a new reserved keyword, because I think it is a common enough English word (and an obscure enough SQL keyword) that people may well have used it for table names or aliases, and so their code will break. There is another thread discussing a similar issue with lead/lag ignore nulls: http://www.postgresql.org/message-id/CAOdE5WQnfR737OkxNXuLWnwpL7=OUssC-63ijP2=2rnrtw8...@mail.gmail.com Playing around with the idea proposed there, it seems that it is possible to make FILTER (and also OVER) unreserved keywords, by splitting out the production of aggregate/window functions from normal functions in gram.y. Aggregate/window functions are not allowed in index_elem or func_table constructs, but they may appear in c_expr's. That resolves the shift/reduce conflicts that would otherwise occur from subsequent alias clauses, allowing FILTER and OVER to be unreserved. There is a drawback --- certain error messages become less specific, for example: "SELECT * FROM rank() OVER(ORDER BY random());" is now a syntax error, rather than the more useful error saying that window functions aren't allowed in the FROM clause. That doesn't seem like such a common case though. What do you think? Regards, Dean make-filter-unreserved.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] A better way than tweaking NTUP_PER_BUCKET
On Sunday, June 23, 2013, Simon Riggs wrote: > On 23 June 2013 03:16, Stephen Frost > > wrote: > > > Will think on it more. > > Some other thoughts related to this... > > * Why are we building a special kind of hash table? Why don't we just > use the hash table code that we in every other place in the backend. > If that code is so bad why do we use it everywhere else? That is > extensible, so we could try just using that. (Has anyone actually > tried?) I've not looked at the hash table in the rest of the backend. > * We're not thinking about cache locality and set correspondence > either. If the join is expected to hardly ever match, then we should > be using a bitmap as a bloom filter rather than assuming that a very > large hash table is easily accessible. That's what I was suggesting earlier, though I don't think it's technically a bloom filter- doesn't that require multiple hash functions?I don't think we want to require every data type to provide multiple hash functions. > * The skew hash table will be hit frequently and would show good L2 > cache usage. I think I'll try adding the skew table always to see if > that improves the speed of the hash join. > The skew tables is just for common values though... To be honest, I have some doubts about that structure really being a terribly good approach for anything which is completely in memory. Thanks, Stephen
Re: [HACKERS] A better way than tweaking NTUP_PER_BUCKET
On Sunday, June 23, 2013, Simon Riggs wrote: > On 23 June 2013 03:16, Stephen Frost > > wrote: > > > Still doesn't really address the issue of dups though. > > Checking for duplicates in all cases would be wasteful, since often we > are joining to the PK of a smaller table. Well, that's what ndistinct is there to help us figure out. If we don't trust that though... > If duplicates are possible at all for a join, then it would make sense > to build the hash table more carefully to remove dupes. I think we > should treat that as a separate issue. > We can't simply remove the dups... We have to return all the matching dups in the join. I did write a patch which created a 2-level list structure where the first level was uniques and the 2nd was dups, but it was extremely expensive to create the hash table then and scanning it wasn't much cheaper. Thanks, Stephen
[HACKERS] INTERVAL overflow detection is terribly broken
Hi, after studying ITERVAL and having a long chat with RhoidumToad and StuckMojo on #postgresql, I am presenting you 3 bugs regarding INTERVAL. As far as I understand, the Interval struct (binary internal representation) consists of: int32 months int32 days int64 microseconds 1. OUTPUT ERRORS: Since 2^63 microseconds equals 2,562,047,788 > 2^31 hours, the overflow in pg_tm when displaying the value causes overflow. The value of Interval struct is actually correct, error happens only on displaying it. SELECT INTERVAL '2147483647 hours' + INTERVAL '5 hours' "-2147483644:00:00" Even wireder: SELECT INTERVAL '2147483647 hours' + '1 hour' "--2147483648:00:00" notice the double minus? Don't ask how I came across this two bugs. 2. OPERATION ERRORS: When summing two intervals, the user is not notified when overflow occurs: SELECT INT '2147483647' + INT '1' ERROR: integer out of range SELECT INTERVAL '2147483647 days' + INTERVAL '1 day' "-2147483648 days" This should be analogous. 3. PARSER / INPUT ERRORS: This is perhaps the hardest one to explain, since this is an architectural flaw. You are checking the overflows when parsing string -> pg_tm struct. However, at this point, the parser doesn't know, that weeks and days are going to get combined, or years are going to get converted to months, for example. Unawarness of underlying Interval struct causes two types of suberrors: a) False positive SELECT INTERVAL '2147483648 microseconds' ERROR: interval field value out of range: "2147483648 microseconds" This is not right. Microseconds are internally stored as 64 bit signed integer. The catch is: this amount of microseconds is representable in Interval data structure, this shouldn't be an error. b) False negative SELECT INTERVAL '10 years' "-73741824 years" We don't catch errors like this, because parser only checks for overflow in pg_tm. If overflow laters happens in Interval, we don't seem to care. 4. POSSIBLE SOLUTIONS: a) Move the overflow checking just after the conversion of pg_tm -> Interval is made. This way, you can accurately predict if the result is really not store-able. b) Because of 1), you have to declare tm_hour as int64, if you want to use that for the output. But, why not use Interval struct for printing directly, without intermediate pg_tm? 5. Offtopic: Correct the documentation, INTERVAL data size is 16 bytes, not 12. Rok Kralj -- eMail: rok.kr...@gmail.com
Re: [HACKERS] A better way than tweaking NTUP_PER_BUCKET
On 23 June 2013 03:16, Stephen Frost wrote: > Will think on it more. Some other thoughts related to this... * Why are we building a special kind of hash table? Why don't we just use the hash table code that we in every other place in the backend. If that code is so bad why do we use it everywhere else? That is extensible, so we could try just using that. (Has anyone actually tried?) * We're not thinking about cache locality and set correspondence either. If the join is expected to hardly ever match, then we should be using a bitmap as a bloom filter rather than assuming that a very large hash table is easily accessible. * The skew hash table will be hit frequently and would show good L2 cache usage. I think I'll try adding the skew table always to see if that improves the speed of the hash join. -- Simon Riggs 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] Patch for fast gin cache performance improvement
I just tried it and my account works now. Thanks! Ian Stefan Kaltenbrunner Sunday, June 23, 2013 2:05 AM have you tried to log in once to the main website per:http://www.postgresql.org/message-id/CABUevEyt9tQfcF7T2Uzcr8WeF9M=s8qSACuCmN5L2Et26=r...@mail.gmail.com?Stefan ian link Saturday, June 22, 2013 7:03 PM Looks like my community login is still not working. No rush, just wanted to let you know. Thanks!Ian Ian Link Tuesday, June 18, 2013 11:41 AM No worries! I'll just wait until it's up again. Thanks Ian Kevin Grittner Tuesday, June 18, 2013 9:15 AM Oops -- we seem to have a problem with new community logins at themoment, which will hopefully be straightened out soon. You mightwant to wait a few days if you don't already have a login.--Kevin GrittnerEnterpriseDB: http://www.enterprisedb.comThe Enterprise PostgreSQL Company Kevin Grittner Tuesday, June 18, 2013 9:09 AM Ian Link wrote: This patch contains a performance improvement for the fast gin cache. Our test queries improve from about 0.9 ms to 0.030 ms. Impressive! Thanks for reading and considering this patch! Congratulations on your first PostgreSQL patch! To get it scheduled for review, please add it to this page: https://commitfest.postgresql.org/action/commitfest_view/open You will need to get a community login (if you don't already have one), but that is a quick and painless process. Choose an appropriate topic (like "Performance") and reference the message ID of the email to which you attached the patch. Don't worry about the fields for reviewers, committer, or date closed. Sorry for the administrative overhead, but without it things can fall through the cracks. You can find an overview of the review process with links to more detail here: http://wiki.postgresql.org/wiki/CommitFest Thanks for contributing!
Re: [HACKERS] Patch for fast gin cache performance improvement
On 06/23/2013 04:03 AM, ian link wrote: > Looks like my community login is still not working. No rush, just wanted > to let you know. Thanks! have you tried to log in once to the main website per: http://www.postgresql.org/message-id/CABUevEyt9tQfcF7T2Uzcr8WeF9M=s8qSACuCmN5L2Et26=r...@mail.gmail.com ? Stefan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pgbench --throttle (submission 7 - with lag measurement)
So my argumented conclusion is that the issue is somewhere within PQfinish(), possibly in interaction with pgbench doings, but is *NOT* related in any way to the throttling patch, as it is preexisting it. Gregs stumbled upon it because he looked at latencies. An additional thought: The latency measures *elapsed* time. As a small laptop is running 30 clients and their server processes at a significant load, there are a lot of context switching going on, so maybe it just happens that the pgbench process is switched off and then on as PQfinish() is running, so the point would really be that the host is loaded and that's it. I'm not sure of the likelyhood of such an event. It is possible that would be more frequent after timer_exceeded because the system is closing postgres processes, and would depend on what the kernel process scheduler does. So the explanation would be: your system is loaded, and it shows in subtle ways here and there when you do detailed measures. That is life. Basically this is a summary my (long) experience with performance experiments on computers. What are you really measuring? What is really happening? When a system is loaded, there are many things which interact one with the other and induce particular effects on performance measures. So usually what is measured is not what one is expecting. Greg thought that he was measuring transaction latencies, but it was really client contention in a thread. I thought that I was measuring PQfinish() time, but maybe it is the probability of a context switch. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
