[HACKERS] triggers and inheritance tree
Hi, i was trying to create triggers that redirect INSERT/UPDATE/DELETE actions from parent to childs, but found that UPDATE/DELETE doesn't get redirected. Actually, the triggers BEFORE UPDATE and BEFORE DELETE aren't even fired. I haven't tried with AFTER triggers to see if they are fired but i tried on 8.4 to 9.1 and all of these have the same behaviour attached is a simple contained test of this PS: i'm hoping this is just me needed to sleep -- Jaime Casanova www.2ndQuadrant.com Professional PostgreSQL: Soporte 24x7 y capacitación drop database if exists test; create database test; \c test create language plpgsql; create schema schema1; create schema schema2; create table t1(i int primary key, t text); create table schema1.t1() inherits (public.t1); create table schema2.t1() inherits (public.t1); create function f_insert_trigger() returns trigger as $$ begin raise notice 'trigger on %', TG_OP; insert into schema1.t1 values(new.i, new.t); return null; end; $$ language plpgsql; create function f_update_trigger() returns trigger as $$ begin raise notice 'trigger on %', TG_OP; update schema1.t1 set i = new.i, t = new.t where i = old.i and t = old.t; return null; end; $$ language plpgsql; create function f_delete_trigger() returns trigger as $$ begin raise notice 'trigger on %', TG_OP; delete from schema1.t1 where i = old.i and t = old.t; return null; end; $$ language plpgsql; create trigger trg_insert before insert on public.t1 for each row execute procedure f_insert_trigger(); create trigger trg_update before update on public.t1 for each row execute procedure f_update_trigger(); create trigger trg_delete before delete on public.t1 for each row execute procedure f_delete_trigger(); -- insert some data insert into schema1.t1 values(1, 'test'); insert into schema2.t1 values(2, 'test'); insert into schema1.t1 values(3, 'fixed row'); -- test triggers -- ok, this one is redirected insert into t1 values(4, 'redirected'); select * from t1; select * from schema1.t1; -- bad, update and delete don't respect the triggers update t1 set t = t || ' updated' where t = 'test'; select * from t1; delete from t1 where t = 'test updated'; select * from t1; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgsql_fdw, FDW for PostgreSQL server
2012/3/28 Shigeru HANADA shigeru.han...@gmail.com: (2012/03/27 20:32), Thom Brown wrote: 2012/3/26 Shigeru HANADAshigeru.han...@gmail.com: * pgsql_fdw_v17.patch - Adds pgsql_fdw as contrib module * pgsql_fdw_pushdown_v10.patch - Adds WHERE push down capability to pgsql_fdw * pgsql_fdw_analyze_v1.patch - Adds pgsql_fdw_analyze function for updating local stats Hmm... I've applied them using the latest Git master, and in the order specified, but I can't build the contrib module. What am I doing wrong? I'm sorry, but I couldn't reproduce the errors with this procedure. $ git checkout master $ git pull upstream master # make master branch up-to-date $ git clean -fd # remove files for other branches $ make clean # just in case $ patch -p1 /path/to/pgsql_fdw_v17.patch $ patch -p1 /path/to/pgsql_fdw_pushdown_v10.patch $ patch -p1 /path/to/pgsql_fdw_analyze_v1.patch $ make # make core first for libpq et al. $ cd contrib/pgsql_fdw $ make # pgsql_fdw Please try git clean and make clean, if you have not. FWIW, I'm using GNU Make 3.82 and gcc 4.6.0 on Fedora 15. I had done a make clean, git stash and git clean -f, but I didn't try git clean -fd. For some reason it's working now. Thanks -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgsql_fdw, FDW for PostgreSQL server
On 28 March 2012 08:13, Thom Brown t...@linux.com wrote: 2012/3/28 Shigeru HANADA shigeru.han...@gmail.com: (2012/03/27 20:32), Thom Brown wrote: 2012/3/26 Shigeru HANADAshigeru.han...@gmail.com: * pgsql_fdw_v17.patch - Adds pgsql_fdw as contrib module * pgsql_fdw_pushdown_v10.patch - Adds WHERE push down capability to pgsql_fdw * pgsql_fdw_analyze_v1.patch - Adds pgsql_fdw_analyze function for updating local stats Hmm... I've applied them using the latest Git master, and in the order specified, but I can't build the contrib module. What am I doing wrong? I'm sorry, but I couldn't reproduce the errors with this procedure. $ git checkout master $ git pull upstream master # make master branch up-to-date $ git clean -fd # remove files for other branches $ make clean # just in case $ patch -p1 /path/to/pgsql_fdw_v17.patch $ patch -p1 /path/to/pgsql_fdw_pushdown_v10.patch $ patch -p1 /path/to/pgsql_fdw_analyze_v1.patch $ make # make core first for libpq et al. $ cd contrib/pgsql_fdw $ make # pgsql_fdw Please try git clean and make clean, if you have not. FWIW, I'm using GNU Make 3.82 and gcc 4.6.0 on Fedora 15. I had done a make clean, git stash and git clean -f, but I didn't try git clean -fd. For some reason it's working now. Hmm.. I'm getting some rather odd errors though: thom@test=# select * from stuff limit 3 ; DEBUG: StartTransactionCommand DEBUG: StartTransaction DEBUG: name: unnamed; blockState: DEFAULT; state: INPROGR, xid/subid/cid: 0/1/0, nestlvl: 1, children: LOG: statement: select * from stuff limit 3 ; DEBUG: relid=16402 fetch_count=1 DEBUG: Remote SQL: SELECT id, stuff, age FROM public.stuff DEBUG: starting remote transaction with START TRANSACTION ISOLATION LEVEL REPEATABLE READ ERROR: could not declare cursor DETAIL: ERROR: relation public.stuff does not exist LINE 1: ...or_6 SCROLL CURSOR FOR SELECT id, stuff, age FROM public.stu... ^ HINT: DECLARE pgsql_fdw_cursor_6 SCROLL CURSOR FOR SELECT id, stuff, age FROM public.stuff The table in question indeed doesn't exist, but I'm confused as to why the user is being exposed to such messages. And more troublesome: (local select on foreign server): test=# select * from stuff limit 3 ; id | thing | age +--+- 1 | STANDARD | 30 2 | STANDARD | 29 3 | STANDARD | 12 (3 rows) (foreign select on foreign server): thom@test=# select * from stuff limit 3 ; id | stuff | age +-+- 1 | (1,STANDARD,30) | 30 2 | (2,STANDARD,29) | 29 3 | (3,STANDARD,12) | 12 (3 rows) The row expansion seems to incorrectly rewrite the column without a table prefix if both column and table name are identical. -- Thom -- 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] Command Triggers patch v18
Robert Haas robertmh...@gmail.com writes: I think BEFORE command triggers ideally should run * before permission checks * before locking * before internal checks are done (nameing conflicts, type checks and such) It is possible to do this, and it would actually be much easier and less invasive to implement than what Dimitri has done here, but the downside is that you won't have done the name lookup either. There's a trade-off decision to take here, that was different in previous versions of the patch. You can either run the trigger very early or have specific information details. The way to have both and keep your sanity, and that was implemented in the patch, is to have ANY command triggers run before the process utility big switch and provide only the command tag and parse tree, and have the specific triggers called after permission, locking and internal checks are done. I've been asked to have a single call site for ANY and specific triggers, which means you can't have BEFORE triggers running either before or after those steps. Now I can see why implementing that on top of the ANY command feature is surprising enough for wanting to not do it this way. Maybe the answer is to use another keyword to be able to register command triggers that run that early and without any specific information other than the command tag. Regards, -- 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] Re: pg_stat_statements normalisation without invasive changes to the parser (was: Next steps on pg_stat_statements normalisation)
[ also for the archives' sake ] On 27 March 2012 22:05, Tom Lane t...@sss.pgh.pa.us wrote: Well, testing function pointers for null is certainly OK --- note that all our hook function call sites do that. It's true that testing for equality to a particular function's name can fail on some platforms because of jump table hacks. I was actually talking about stylistic iffiness. This seems contrary to the standard, which states: (ISO C 99 section 6.5.9.6) Two pointers compare equal if and only if both are null pointers, both are pointers to the same object (...) or function, both are pointers to one past the last element of the same array object, or one is a pointer to one past the end of one array object and the other is a pointer to the start of a different array object that happens to immediately follow the first array object in the address space. However, the fly in the ointment is IA-64 (Itanic), which apparently at least at one stage had broken function pointer comparisons, at least when code was built using some version(s) of GCC. I found it a bit difficult to square your contention that performing function pointer comparisons against function addresses was what sounded like undefined behaviour, and yet neither GCC nor Clang complained. However, in light of what I've learned about IA-64, I can certainly see why we as a project would avoid the practice. Source: http://gcc.gnu.org/ml/gcc/2003-06/msg01283.html -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and 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] poll: CHECK TRIGGER?
Ok, seems that the API issue is settled, so I'm now looking at the code actually doing the checking. My first impression is that this is a lot of code. Can we simplify it? Since this is deeply integrated into the PL/pgSQL interpreter, I was expecting that this would run through the normal interpreter, in a special mode that skips all the actual execution of queries, and shortcuts all loops and other control statements so that all code is executed only once. That would mean sprinkling some if (check_only) code into the normal exec_* functions. I'm not sure how invasive that would be, but it's worth considering. I think you would be able to more easily catch more errors that way, and the check code would stay better in sync with the execution code. Another thought is that check_stmt() and all its subroutines are very similar to the plpgsql_dumptree() code. Would it make sense to merge those? You could have an output mode, in addition to the xml and plain-text formats, that would just dump the whole tree like plpgsql_dumptree() does. In prepare_expr(), you use a subtransaction to catch any ERRORs that happen during parsing the expression. That's a good idea, and I think many of the check_* functions could be greatly simplified by adopting a similar approach. Just ereport() any errors you find, and catch them at the appropriate level, appending the error to the output string. Your current approach of returning true/false depending on whether there was any errors seems tedious. If you create a function with an invalid body (ie. set check_function_bodies=off; create function ... $$ bogus $$;) , plpgsql_check_function() still throws an error. It's understandable that it cannot do in-depth analysis if the function cannot be parsed, but I would expect the syntax error to be returned as a return value like other errors that it complains about, not thrown as a hard ERROR. That would make it more useful to bulk-check all functions in a database with something like select plpgsql_check_function(oid) from pg_class. As it is, the checking stops at the first invalid function with an error. PS. I think plpgsql_check_function() belongs in pl_handler.c -- Heikki Linnakangas 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
[HACKERS] Feature proposal: list role members in psql
It seems there is no way to get role members using psql. \du and \dg
give you this role is a member of information, but no information
about who have been granted the role.
I would like to write a patch implementing this feature. Some questions:
- Is this wanted?
- Should there be a new flag for this: \du[m+] where m list the role
memebers. Or could this be added to the '+' flag?
- Any pointers for patches implementing a similar addition to psql?
That would help me a lot, as I do not know the code base.
- Should the roles be fetched recursively, or should it be just the
direct role members? My opinion is that direct role members is what is
wanted.
The output would be something like:
Role name | Attributes | Member of| Role members
---+--++---
hot2_dba | Cannot login | {hot2_user,common_dba} | {akaariai}
- Anssi Kääriäinen
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgsql_fdw, FDW for PostgreSQL server
On 28 March 2012 08:39, Thom Brown t...@linux.com wrote: On 28 March 2012 08:13, Thom Brown t...@linux.com wrote: 2012/3/28 Shigeru HANADA shigeru.han...@gmail.com: (2012/03/27 20:32), Thom Brown wrote: 2012/3/26 Shigeru HANADAshigeru.han...@gmail.com: * pgsql_fdw_v17.patch - Adds pgsql_fdw as contrib module * pgsql_fdw_pushdown_v10.patch - Adds WHERE push down capability to pgsql_fdw * pgsql_fdw_analyze_v1.patch - Adds pgsql_fdw_analyze function for updating local stats Hmm... I've applied them using the latest Git master, and in the order specified, but I can't build the contrib module. What am I doing wrong? I'm sorry, but I couldn't reproduce the errors with this procedure. $ git checkout master $ git pull upstream master # make master branch up-to-date $ git clean -fd # remove files for other branches $ make clean # just in case $ patch -p1 /path/to/pgsql_fdw_v17.patch $ patch -p1 /path/to/pgsql_fdw_pushdown_v10.patch $ patch -p1 /path/to/pgsql_fdw_analyze_v1.patch $ make # make core first for libpq et al. $ cd contrib/pgsql_fdw $ make # pgsql_fdw Please try git clean and make clean, if you have not. FWIW, I'm using GNU Make 3.82 and gcc 4.6.0 on Fedora 15. I had done a make clean, git stash and git clean -f, but I didn't try git clean -fd. For some reason it's working now. Hmm.. I'm getting some rather odd errors though: thom@test=# select * from stuff limit 3 ; DEBUG: StartTransactionCommand DEBUG: StartTransaction DEBUG: name: unnamed; blockState: DEFAULT; state: INPROGR, xid/subid/cid: 0/1/0, nestlvl: 1, children: LOG: statement: select * from stuff limit 3 ; DEBUG: relid=16402 fetch_count=1 DEBUG: Remote SQL: SELECT id, stuff, age FROM public.stuff DEBUG: starting remote transaction with START TRANSACTION ISOLATION LEVEL REPEATABLE READ ERROR: could not declare cursor DETAIL: ERROR: relation public.stuff does not exist LINE 1: ...or_6 SCROLL CURSOR FOR SELECT id, stuff, age FROM public.stu... ^ HINT: DECLARE pgsql_fdw_cursor_6 SCROLL CURSOR FOR SELECT id, stuff, age FROM public.stuff The table in question indeed doesn't exist, but I'm confused as to why the user is being exposed to such messages. And more troublesome: (local select on foreign server): test=# select * from stuff limit 3 ; id | thing | age +--+- 1 | STANDARD | 30 2 | STANDARD | 29 3 | STANDARD | 12 (3 rows) (foreign select on foreign server): thom@test=# select * from stuff limit 3 ; id | stuff | age +-+- 1 | (1,STANDARD,30) | 30 2 | (2,STANDARD,29) | 29 3 | (3,STANDARD,12) | 12 (3 rows) The row expansion seems to incorrectly rewrite the column without a table prefix if both column and table name are identical. Actually, correction. The foreign table definition names the column the same as the table. I accidentally omitted the 'thing' column and instead substituted it with the table name in the definition. Original table definition on foreign server: create table stuff (id serial primary key, thing text, age int); Foreign table definition: create foreign table stuff (id int not null, stuff text, age int) server pgsql; So it appears I'm allowed to use the table as a column in this context. So please disregard my complaint. -- Thom -- 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] Command Triggers patch v18
On Wed, Mar 28, 2012 at 4:12 AM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Robert Haas robertmh...@gmail.com writes: I think BEFORE command triggers ideally should run * before permission checks * before locking * before internal checks are done (nameing conflicts, type checks and such) It is possible to do this, and it would actually be much easier and less invasive to implement than what Dimitri has done here, but the downside is that you won't have done the name lookup either. There's a trade-off decision to take here, that was different in previous versions of the patch. You can either run the trigger very early or have specific information details. The way to have both and keep your sanity, and that was implemented in the patch, is to have ANY command triggers run before the process utility big switch and provide only the command tag and parse tree, and have the specific triggers called after permission, locking and internal checks are done. I've been asked to have a single call site for ANY and specific triggers, which means you can't have BEFORE triggers running either before or after those steps. Now I can see why implementing that on top of the ANY command feature is surprising enough for wanting to not do it this way. Maybe the answer is to use another keyword to be able to register command triggers that run that early and without any specific information other than the command tag. Yeah, I think so. I objected to the way you had it because of the inconsistency, not because I think it's a useless place to fire triggers. -- 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] Command Triggers patch v18
On Wed, Mar 28, 2012 at 7:16 AM, Robert Haas robertmh...@gmail.com wrote: Now I can see why implementing that on top of the ANY command feature is surprising enough for wanting to not do it this way. Maybe the answer is to use another keyword to be able to register command triggers that run that early and without any specific information other than the command tag. Yeah, I think so. I objected to the way you had it because of the inconsistency, not because I think it's a useless place to fire triggers. Further thought: I think maybe we shouldn't use keywords at all for this, and instead use descriptive strings like post-parse or pre-execution or command-start, because I bet in the end we're going to end up with a bunch of them (create-schema-subcommand-start? alter-table-subcommand-start?), and it would be nice not to hassle with the grammar every time we want to add a new one. To make this work with the grammar, we could either (1) enforce that each is exactly one word or (2) require them to be quoted or (3) require those that are not a single word to be quoted. I tend think #2 might be the best option in this case, but... I've also had another general thought about safety: we need to make sure that we're only firing triggers from places where it's safe for user code to perform arbitrary actions. In particular, we have to assume that any triggers we invoke will AcceptInvalidationMessages() and CommandCounterIncrement(); and we probably can't do it at all (or at least not without some additional safeguard) in places where the code does CheckTableNotInUse() up through the point where it's once again safe to access the table, since the trigger might try. We also need to think about re-entrancy: that is, in theory, the trigger might execute any other DDL command - e.g. it might drop the table that we've been asked to rename. I suspect that some of the current BEFORE trigger stuff might fall down on that last one, since the existing code not-unreasonably expects that once it's locked the table, the catalog entries can't go away. Maybe it all happens to work out OK, but I don't think we can count on that. -- 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
[HACKERS] Re: [COMMITTERS] pgsql: pg_test_timing utility, to measure clock monotonicity and timing
On Tue, Mar 27, 2012 at 10:10 PM, Fujii Masao masao.fu...@gmail.com wrote: On Wed, Mar 28, 2012 at 5:17 AM, Robert Haas rh...@postgresql.org wrote: pg_test_timing utility, to measure clock monotonicity and timing cost. When I compiled this, I got a compiler warning. Attached patch silences the warning. Unfortunately, that *produces* a warning on my machine. Normally, I think we handle this using INT64_FORMAT, but the fact that it's %10ld here and not just %lld makes that awkward. I guess we maybe need to insert some kludgy workaround here - write it into a separate buffer, and then blank-pad it, or something like that. Also I found one trivial problem in the doc of pg_test_timing. The patch fixes that. Thanks, committed. -- 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] pgsql_fdw, FDW for PostgreSQL server
(2012/03/28 16:18), Albe Laurenz wrote: I wrote: How about getting # of rows estimate by executing EXPLAIN for fully-fledged remote query (IOW, contains pushed-down WHERE clause), and estimate selectivity of local filter on the basis of the statistics which are generated by FDW via do_analyze_rel() and FDW-specific sampling function? In this design, we would be able to use quite correct rows estimate because we can consider filtering stuffs done on each side separately, though it requires expensive remote EXPLAIN for each possible path. That sounds nice. ... but it still suffers from the problems of local statistics for remote tables I pointed out. I guess that you mean about these issues you wrote in earlier post, so I'd like to comment on them. - You have an additional maintenance task if you want to keep statistics for remote tables accurate. I understand that this may get better in a future release. I'm not sure that's what you meant, but we need to execute remote ANALYZE before calling pgsql_fdw_analyze() to keep local statistics accurate. IMO DBAs are responsible to execute remote ANALYZE at appropriate timing, so pgsql_fdw_analyze (or handler function for ANALYZE) should just collect statistics from remote side. - You depend on the structure of pg_statistic, which means a potential incompatibility between server versions. You can add cases to pgsql_fdw_analyze to cater for changes, but that is cumbersome and will only help for later PostgreSQL versions connecting to earlier ones. Indeed. Like pg_dump, pgsql_fdw should aware of different server version if we choose copying statistics. Difference of catalog structure is very easy to track and cope with, but if meanings of values or the way to calculate statistics are changed, pgsql_fdw would need very complex codes to convert values from different version. I don't know such example, but IMO we should assume that statistics are valid for only same version (at least major version). After all, I'd prefer collecting sample data by pgsql_fdw and leaving statistics generation to local backend. - Planning and execution will change (improve, of course) between server versions. The local planner may choose an inferior plan based on a wrong assumption of how a certain query can be handled on the remote. Hm, I don't worry about detail of remote planning so much, because remote server would do its best for a query given by pgsql_fdw. Also local planner would do its best for given estimation (rows, width and costs). One concern is that remote cost factors might be different from local's, so FDW option which represents cost conversion coefficient (1.0 means that remote cost has same weight as local) might be useful. - You have no statistics if the foreign table points to a view on the remote system. ISTM that this would be enough reason to give up copying remote stats to local. We don't provide SELECT push-down nor GROUP BY push-down at present, so users would want to create views which contain function call in SELECT clauses. I think that these shortcomings are not justified by the gain of one client-server round trip less during planning. I'd prefer if pgsql_fdw were not dependent on remote statistics stored in the local database. I too prefer if pgsql_fdw doesn't fully depend on statistics of foreign data, but IMO having statistics of foreign data which were calculated in the way same as local data seems still useful for estimation about local filtering. Even if we have no statistics of foreign data on local side, still we would be able to create plans on the basis of default selectivity for each expression, as same as regular tables. Regards, -- Shigeru HANADA -- 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: pg_basebackup (missing exit on error)
On Tue, Mar 27, 2012 at 7:13 AM, Fujii Masao masao.fu...@gmail.com wrote:
Attached patch removes the fflush() part, changes the log message and removes
the check of tarfile, as above.
With this patch applied, we end up with:
if (strcmp(basedir, -) == 0)
{
#ifdef HAVE_LIBZ
if (ztarfile != NULL)
gzclose(ztarfile);
#endif
}
else
{
#ifdef HAVE_LIBZ
if (ztarfile != NULL)
gzclose(ztarfile);
else
#endif
{
if (fclose(tarfile) != 0)
{
fprintf(stderr, _(%s: could not close file
\%s\: %s\n),
progname, filename, strerror (errno));
disconnect_and_exit(1);
}
}
}
I think it would make sense to rearrange that so that we don't have
two tests for ztarfile != NULL; do that test first, and then if it
fails, do the strcmp after that.
Also, if we're going to test the return value of fclose(), shouldn't
we also be checking the return value of gzclose()?
--
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] Improvement of log messages in pg_basebackup
On Tue, Mar 27, 2012 at 5:25 AM, Fujii Masao masao.fu...@gmail.com wrote:
fprintf(stderr, _(%s: could not identify system: %s\n),
progname, PQerrorMessage(conn));
Since PQerrorMessage() result includes a trailing newline, the above
log message in pg_basebackup doesn't need to include a trailing \n.
Attached patch gets rid of that \n.
res = PQgetResult(conn);
if (PQresultStatus(res) != PGRES_TUPLES_OK)
{
fprintf(stderr, _(%s: could not get WAL end position from
server\n),
progname);
ISTM it's worth including PQerrorMessage() result in the above log
message, to diagnose the cause of error. Attached patch does that.
Committed.
--
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] Re: [COMMITTERS] pgsql: pg_test_timing utility, to measure clock monotonicity and timing
On Wed, Mar 28, 2012 at 08:19:37AM -0400, Robert Haas wrote: On Tue, Mar 27, 2012 at 10:10 PM, Fujii Masao masao.fu...@gmail.com wrote: On Wed, Mar 28, 2012 at 5:17 AM, Robert Haas rh...@postgresql.org wrote: pg_test_timing utility, to measure clock monotonicity and timing cost. When I compiled this, I got a compiler warning. Attached patch silences the warning. Unfortunately, that *produces* a warning on my machine. Normally, I think we handle this using INT64_FORMAT, but the fact that it's %10ld here and not just %lld makes that awkward. I guess we maybe need to insert some kludgy workaround here - write it into a separate buffer, and then blank-pad it, or something like that. How about: .. %10 INT64_FORMAT .. ? -- marko -- 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: [COMMITTERS] pgsql: pg_test_timing utility, to measure clock monotonicity and timing
On Wed, Mar 28, 2012 at 03:46:26PM +0300, Marko Kreen wrote: On Wed, Mar 28, 2012 at 08:19:37AM -0400, Robert Haas wrote: On Tue, Mar 27, 2012 at 10:10 PM, Fujii Masao masao.fu...@gmail.com wrote: On Wed, Mar 28, 2012 at 5:17 AM, Robert Haas rh...@postgresql.org wrote: pg_test_timing utility, to measure clock monotonicity and timing cost. When I compiled this, I got a compiler warning. Attached patch silences the warning. Unfortunately, that *produces* a warning on my machine. Normally, I think we handle this using INT64_FORMAT, but the fact that it's %10ld here and not just %lld makes that awkward. I guess we maybe need to insert some kludgy workaround here - write it into a separate buffer, and then blank-pad it, or something like that. How about: .. %10 INT64_FORMAT .. ? Well, it won't work because unlike inttypes.h, Postgres *_FORMAT includes '%' in it. I guess that why inttypes.h does not do it... -- marko -- 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: [COMMITTERS] pgsql: pg_test_timing utility, to measure clock monotonicity and timing
On Wed, Mar 28, 2012 at 8:51 AM, Marko Kreen mark...@gmail.com wrote: How about: .. %10 INT64_FORMAT .. ? Well, it won't work because unlike inttypes.h, Postgres *_FORMAT includes '%' in it. I guess that why inttypes.h does not do it... Hmm, I guess we could change that, but it would create a hazard for thirty-party code that wants to be cross-version, and for back-patching. We could work around that by doing something more complex, like creating additional symbols, but I'm thinking it ain't worth it just for this. -- 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] pgsql_fdw, FDW for PostgreSQL server
(2012/03/28 21:07), Albe Laurenz wrote: I found another limitation of this approach: pgsql_fdw_analyze() has to run as a user who can update pg_statistic, and this user needs a user mapping to a remote user who can read pg_statistic. This is not necessary for normal operation and needs to be configured specifically for getting remote statistics. This is cumbersome, and people might be unhappy to have to create user mappings for highly privileged remote users. Agreed. After all, supporting ANALYZE command for foreign tables seems the only way to obtain local statistics of foreign data without granting privileges too much. ANALYZE is allowed to only the owner of the table or a superuser, so assuming that an invoker has valid user mapping for a remote user who can read corresponding foreign data seems reasonable. ANALYZE support for foreign tables is proposed by Fujita-san in current CF, so I'd like to push it. Regards, -- Shigeru HANADA -- 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: [COMMITTERS] pgsql: pg_test_timing utility, to measure clock monotonicity and timing
On Wed, Mar 28, 2012 at 08:57:42AM -0400, Robert Haas wrote: On Wed, Mar 28, 2012 at 8:51 AM, Marko Kreen mark...@gmail.com wrote: How about: .. %10 INT64_FORMAT .. ? Well, it won't work because unlike inttypes.h, Postgres *_FORMAT includes '%' in it. I guess that why inttypes.h does not do it... Hmm, I guess we could change that, but it would create a hazard for thirty-party code that wants to be cross-version, and for back-patching. We could work around that by doing something more complex, like creating additional symbols, but I'm thinking it ain't worth it just for this. Changing existing definition is bad idea indeed. And long-term path should be to move to standard int types, so another custom definition seems counter-productive. (OTOH, the 2 int64 _FORMATs are the only formats we maintain.) In this case the simple approach would be to use 'long long': .. %10lld .., (long long)(..) At least ecpg code uses it freely, and nobody has complained, so I guess we don't have any platforms that do not have it. -- marko -- 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] triggers and inheritance tree
On Wed, Mar 28, 2012 at 1:21 AM, Jaime Casanova ja...@2ndquadrant.com wrote: Hi, i was trying to create triggers that redirect INSERT/UPDATE/DELETE actions from parent to childs, but found that UPDATE/DELETE doesn't get redirected. Actually, the triggers BEFORE UPDATE and BEFORE DELETE aren't even fired. and of course, it has nothing to do with the inheritance tree. that was just a coincidence. the problem occurs the same with normal tables, but i can't find where is the problem. i suspect, though, that is in the comparison in TRIGGER_TYPE_MATCHES -- Jaime Casanova www.2ndQuadrant.com Professional PostgreSQL: Soporte 24x7 y capacitación -- 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] triggers and inheritance tree
On Wed, Mar 28, 2012 at 9:16 AM, Jaime Casanova ja...@2ndquadrant.com wrote: On Wed, Mar 28, 2012 at 1:21 AM, Jaime Casanova ja...@2ndquadrant.com wrote: Hi, i was trying to create triggers that redirect INSERT/UPDATE/DELETE actions from parent to childs, but found that UPDATE/DELETE doesn't get redirected. Actually, the triggers BEFORE UPDATE and BEFORE DELETE aren't even fired. and of course, it has nothing to do with the inheritance tree. that was just a coincidence. the problem occurs the same with normal tables, but i can't find where is the problem. i suspect, though, that is in the comparison in TRIGGER_TYPE_MATCHES I think the problem is that the UPDATE or DELETE can only fire once a matching row has been identified, so that OLD can be filled in appropriately. But in this case, the matching row gets found not in the parent table, but in one of its child tables. So any triggers on the child table would fire, but triggers on the parent table will not. -- 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] 9.2 commitfest closure (was Command Triggers, v16)
On Tue, Mar 27, 2012 at 1:36 PM, Robert Haas robertmh...@gmail.com wrote: On Mon, Mar 26, 2012 at 7:39 PM, Tom Lane t...@sss.pgh.pa.us wrote: Fine. What do you propose, specifically? The end of the month is coming up. How about we propose to close the 'fest on April 1st? Anything that's not committable by then goes to the 9.3 list. If one week seems too short, how about 2 weeks? Let's split the difference: how about we close it a week from this Friday. That would be April 6, 2012, ten days from today. Anybody, anybody? Can we try to get some agreement on this? -- 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] Review of pg_archivecleanup -x option patch
On Sun, Mar 11, 2012 at 9:28 PM, Robert Haas robertmh...@gmail.com wrote: On Sat, Mar 10, 2012 at 2:38 PM, Jaime Casanova ja...@2ndquadrant.com wrote: On Fri, Mar 9, 2012 at 9:07 AM, Robert Haas robertmh...@gmail.com wrote: On Fri, Mar 9, 2012 at 12:47 AM, Jaime Casanova ja...@2ndquadrant.com wrote: Sorry, here's the patch rebased and with the suggestion from Alex. Which reminds me, I never thank him for the review (shame on me) :D with the patch this time This may be a stupid idea, but it seems to me that it might be better to dispense with all of the logic in here to detect whether the file name is still going to be long enough after chomping the extension. I feel like that's just making things complicated. while i like the idea of separating the logic, i don't like the results: for example i tried this (notice that i forgot the point), and it just says nothing (not even that the file with the extension but without the point doesn't exist). that's why we were checking that the length matches $ ./pg_archivecleanup -x bz2 /tmp 000100010058 Hmm, but I thought that the idea was that the extension was optional. Perhaps I'm missing something but I don't think the previous patch will complain about that either; or at least I don't see why the behavior should be any different. Can someone enlighten me on this point? -- 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] archive_keepalive_command
On Mon, Mar 5, 2012 at 11:55 AM, Simon Riggs si...@2ndquadrant.com wrote: On Sun, Mar 4, 2012 at 1:20 AM, Jeff Janes jeff.ja...@gmail.com wrote: Does this patch have any user-visible effect? I thought it would make pg_last_xact_replay_timestamp() advance, but it does not seem to. I looked through the source a bit, and as best I can tell this only sets some internal state which is never used, except under DEBUG2 Thanks for the review. I'll look into that. Simon, are you still hoping to get this done for this releases? -- 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] Should I implement DROP INDEX CONCURRENTLY?
On Fri, Feb 3, 2012 at 10:28 AM, Robert Haas robertmh...@gmail.com wrote: On Wed, Feb 1, 2012 at 2:39 PM, Simon Riggs si...@2ndquadrant.com wrote: On Wed, Feb 1, 2012 at 7:31 PM, Peter Eisentraut pete...@gmx.net wrote: On sön, 2012-01-29 at 22:01 +, Simon Riggs wrote: Patch now locks index in AccessExclusiveLock in final stage of drop. Doesn't that defeat the point of doing the CONCURRENTLY business in the first place? That was my initial reaction. We lock the index in AccessExclusiveLock only once we are certain nobody else is looking at it any more. So its a Kansas City Shuffle, with safe locking in case of people doing strange low level things. Yeah, I think this is much safer, and in this version that doesn't seem to harm concurrency. Given our previous experiences in this area, I wouldn't like to bet my life savings on this having no remaining bugs - but if it does, I can't find them. I'll mark this Ready for Committer. I don't think this has been committed - does that mean you've decided against doing so, or just haven't had the round tuits? -- 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] PL/pgSQL return value in after triggers
On Sun, Jan 1, 2012 at 11:37 PM, Peter Eisentraut pete...@gmx.net wrote: One thing that I'm concerned about with this is that it treats a plain RETURN in a BEFORE trigger as RETURN NULL, whereas arguably it should be an error. I haven't found a good way to handle that yet, but I'll keep looking. I would be very much disinclined to change the behavior of BEFORE triggers in this way, so I agree we need a way around that. I'm going to mark this patch Returned with Feedback; I think it's 9.3 material at this point. -- 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] 9.2 commitfest closure (was Command Triggers, v16)
On Wed, Mar 28, 2012 at 2:45 PM, Robert Haas robertmh...@gmail.com wrote: On Tue, Mar 27, 2012 at 1:36 PM, Robert Haas robertmh...@gmail.com wrote: On Mon, Mar 26, 2012 at 7:39 PM, Tom Lane t...@sss.pgh.pa.us wrote: Fine. What do you propose, specifically? The end of the month is coming up. How about we propose to close the 'fest on April 1st? Anything that's not committable by then goes to the 9.3 list. If one week seems too short, how about 2 weeks? Let's split the difference: how about we close it a week from this Friday. That would be April 6, 2012, ten days from today. Anybody, anybody? Can we try to get some agreement on this? I agree. I have a few projects still on the table myself, but my main concern is Alvaro's FK locks patch. Depending on how the bones lie I will finish up some combination of those by end of next week. -- 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] Cross-backend signals and administration (Was: Re: pg_terminate_backend for same-role)
Kevin Grittner kevin.gritt...@wicourts.gov writes: Andres Freund and...@anarazel.de wrote: On Tuesday, March 27, 2012 07:51:59 PM Kevin Grittner wrote: As Tom pointed out, if there's another person sharing the user ID you're using, and you don't trust them, their ability to cancel your session is likely way down the list of concerns you should have. Hm. I don't think that is an entirely valid argumentation. The same user could have entirely different databases. They even could have distinct access countrol via the clients ip. I have seen the same cluster being used for prod/test instances at smaller shops several times. Whether thats a valid usecase I have no idea. Well, that does sort of leave an arguable vulnerability. Should the same user only be allowed to kill the process from a connection to the same database? I don't see a lot of merit in this argument either. If joeseviltwin can connect as joe to database A, he can also connect as joe to database B in the same cluster, and then do whatever damage he wants. Fundamentally, if two users are sharing the same userid, *they are the same user* as far as Postgres is concerned. It's just silly to make protection decisions on the assumption that they might not be. If a DBA does not like the consequences of that, the solution is obvious. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: pg_stat_statements normalisation without invasive changes to the parser (was: Next steps on pg_stat_statements normalisation)
On 27 March 2012 20:26, Tom Lane t...@sss.pgh.pa.us wrote: Have yet to look at the pg_stat_statements code itself. I merged upstream changes with the intention of providing a new patch for you to review. I found a problem that I'd guess was introduced by commit 9dbf2b7d75de5af38d087cbe2b1147dd0fd10f0a, Restructure SELECT INTO's parsetree representation into CreateTableAsStmt. This has nothing to do with my patch in particular. I noticed that my tests broke, on queries like select * into orders_recent FROM orders WHERE orderdate = '2002-01-01';. Since commands like that are now utility statements, I thought it best to just hash the query string directly, along with all other utility statements - richer functionality would be unlikely to be missed all that much, and that's a fairly clean demarcation that I don't want to make blurry. In the existing pg_stat_statements code in HEAD, there are 2 pgss_store call sites - one in pgss_ProcessUtility, and the other in pgss_ExecutorFinish. There is an implicit assumption in the extant code (and my patch too) that there will be exactly one pgss_store call per query execution. However, that assumption appears to now fall down, as illustrated by the GDB session below. What's more, our new hook is called twice, which is arguably redundant. (gdb) break pgss_parse_analyze Breakpoint 1 at 0x7fbd17b96790: file pg_stat_statements.c, line 640. (gdb) break pgss_ProcessUtility Breakpoint 2 at 0x7fbd17b962b4: file pg_stat_statements.c, line 1710. (gdb) break pgss_ExecutorEnd Breakpoint 3 at 0x7fbd17b9618c: file pg_stat_statements.c, line 1674. (gdb) c Continuing. I execute the command select * into orders_recent FROM orders WHERE orderdate = '2002-01-01'; Breakpoint 1, pgss_parse_analyze (pstate=0x2473bc0, post_analysis_tree=0x2474010) at pg_stat_statements.c:640 640 if (post_analysis_tree-commandType != CMD_UTILITY) (gdb) c Continuing. Breakpoint 1, pgss_parse_analyze (pstate=0x2473bc0, post_analysis_tree=0x2474010) at pg_stat_statements.c:640 640 if (post_analysis_tree-commandType != CMD_UTILITY) (gdb) c Continuing. Breakpoint 2, pgss_ProcessUtility (parsetree=0x2473cd8, queryString=0x2472a88 select * into orders_recent FROM orders WHERE orderdate = '2002-01-01';, params=0x0, isTopLevel=1 '\001', dest=0x2474278, completionTag=0x7fff74e481e0 ) at pg_stat_statements.c:1710 1710if (pgss_track_utility pgss_enabled()) (gdb) c Continuing. Breakpoint 3, pgss_ExecutorEnd (queryDesc=0x24c9660) at pg_stat_statements.c:1674 1674if (queryDesc-totaltime pgss_enabled()) (gdb) c Continuing. What do you think we should do about this? -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and 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] Lazy hashaggregate when no aggregation is needed
Ants Aasma a...@cybertec.at writes: A user complained on pgsql-performance that SELECT col FROM table GROUP BY col LIMIT 2; performs a full table scan. ISTM that it's safe to return tuples from hash-aggregate as they are found when no aggregate functions are in use. Attached is a first shot at that. As I commented in the other thread, the user would be a lot better off if he'd had an index on the column in question. I'm not sure it's worth complicating the hashagg logic when an indexscan + groupagg would address the case better. 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] 9.2 commitfest closure (was Command Triggers, v16)
Robert Haas robertmh...@gmail.com writes: On Tue, Mar 27, 2012 at 1:36 PM, Robert Haas robertmh...@gmail.com wrote: Let's split the difference: how about we close it a week from this Friday. That would be April 6, 2012, ten days from today. Anybody, anybody? Can we try to get some agreement on this? Works for me. 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] Cross-backend signals and administration (Was: Re: pg_terminate_backend for same-role)
On Wed, Mar 28, 2012 at 10:02 AM, Tom Lane t...@sss.pgh.pa.us wrote: Kevin Grittner kevin.gritt...@wicourts.gov writes: Andres Freund and...@anarazel.de wrote: On Tuesday, March 27, 2012 07:51:59 PM Kevin Grittner wrote: As Tom pointed out, if there's another person sharing the user ID you're using, and you don't trust them, their ability to cancel your session is likely way down the list of concerns you should have. Hm. I don't think that is an entirely valid argumentation. The same user could have entirely different databases. They even could have distinct access countrol via the clients ip. I have seen the same cluster being used for prod/test instances at smaller shops several times. Whether thats a valid usecase I have no idea. Well, that does sort of leave an arguable vulnerability. Should the same user only be allowed to kill the process from a connection to the same database? I don't see a lot of merit in this argument either. If joeseviltwin can connect as joe to database A, he can also connect as joe to database B in the same cluster, and then do whatever damage he wants. Fundamentally, if two users are sharing the same userid, *they are the same user* as far as Postgres is concerned. It's just silly to make protection decisions on the assumption that they might not be. If a DBA does not like the consequences of that, the solution is obvious. I'm coming around to the point of view that we should just make pg_terminate_backend()'s behavior consistent with pg_cancel_backend() and call it good. -- 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] Publish checkpoint timing and sync files summary data to pg_stat_bgwriter
On Thu, Mar 22, 2012 at 6:07 PM, Peter Geoghegan pe...@2ndquadrant.com wrote: On 23 January 2012 02:08, Robert Haas robertmh...@gmail.com wrote: On Sat, Jan 21, 2012 at 6:32 PM, Jeff Janes jeff.ja...@gmail.com wrote: I'm finding the backend_writes column pretty unfortunate. The only use I know of for it is to determine if the bgwriter is lagging behind. Yet it doesn't serve even this purpose because it lumps together the backend writes due to lagging background writes, and the backend writes by design due to the use buffer access strategy during bulk inserts. +1 for separating those. I decided to have a go myself. Attached patch breaks out strategy allocations in pg_stat_bgwriter, but not strategy writes. My thinking is that this may serve to approximate non-BAS_NORMAL writes, with the considerable advantage of not requiring that I work backwards to figure out strategy from some block when backend-local syncing (yes, syncing, not writing) a buffer to work out which strategy object references the buffer. The bookkeeping that that would likely entail seems to make it infeasible. Incidentally, it seems Postgres doesn't currently record backend writes when the buffer doesn't go on to be sync'd. That seems problematic to me, or at the very least a misrepresentation, since temporary tables will be written out by the backend for example. Not sure if it's worth fixing, though I've added a comment to that effect at the site of where backend_writes is bumped. I have corrected the md.c bug. This previously would have prevented the sync_files (number of relation segments synced) value from being valid in non-log_checkpoints configurations. I'm not currently confident that the strategy_alloc filed is a very useful proxy for a strategy_backend_writes field. I think that rather than bumping the strategy allocation count analogously to the way the overall count is bumped (within StrategyGetBuffer()), I should have bumped earlier within BufferAlloc() so that it'd count if the buffer was requested with non-BAS_NORMAL strategy but was found in shared_buffers (so control never got as far as StrategyGetBuffer() ). That might make the value more closely line-up to the would-be value of a strategy_backend_writes column. What do you think? Although I liked the idea of separating this out, I wasn't thinking we should do it as part of this patch. It seems like an independent project. At any rate, I strongly agree that counting the number of strategy allocations is not really a viable proxy for counting the number of backend writes. You can't know how many of those actually got dirtied. Since any backend write is necessarily the result of that backend trying to allocate a buffer, I think maybe we should just count whether the number of times it was trying to allocate a buffer *using a BAS* vs. the number of times it was trying to allocate a buffer *not using a BAS*. That is, decide whether or not it's a strategy write not based on whether the buffer came in via a strategy allocation, but rather based on whether it's going out as a result of a strategy allocation. -- 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] [PATCH] Support for foreign keys with arrays
Il giorno lun, 19/03/2012 alle 18.41 +0100, Marco Nenciarini ha scritto: Attached is v5, which should address all the remaining issues. Please find attached v6 of the EACH Foreign Key patch. From v5 only cosmetic changes to the documentation were made. Regards, Marco -- Marco Nenciarini - 2ndQuadrant Italy PostgreSQL Training, Services and Support marco.nenciar...@2ndquadrant.it | www.2ndQuadrant.it EACH-foreign-keys.v6.patch.bz2 Description: application/bzip -- 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: pg_stat_statements normalisation without invasive changes to the parser (was: Next steps on pg_stat_statements normalisation)
Peter Geoghegan pe...@2ndquadrant.com writes: I merged upstream changes with the intention of providing a new patch for you to review. I found a problem that I'd guess was introduced by commit 9dbf2b7d75de5af38d087cbe2b1147dd0fd10f0a, Restructure SELECT INTO's parsetree representation into CreateTableAsStmt. This has nothing to do with my patch in particular. Yeah, I already deleted the intoClause chunk from the patch. I think treating SELECT INTO as a utility statement is probably fine, at least for now. In the existing pg_stat_statements code in HEAD, there are 2 pgss_store call sites - one in pgss_ProcessUtility, and the other in pgss_ExecutorFinish. There is an implicit assumption in the extant code (and my patch too) that there will be exactly one pgss_store call per query execution. However, that assumption appears to now fall down, as illustrated by the GDB session below. What's more, our new hook is called twice, which is arguably redundant. That's been an issue right along for cases such as EXPLAIN and EXECUTE, I believe. Perhaps the right thing is to consider such executor calls as nested statements --- that is, the ProcessUtility hook ought to bump the nesting depth too. 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] Cross-backend signals and administration (Was: Re: pg_terminate_backend for same-role)
Robert Haas robertmh...@gmail.com writes: I'm coming around to the point of view that we should just make pg_terminate_backend()'s behavior consistent with pg_cancel_backend() and call it good. Yeah, the issues that are really of concern are not ones that that function in itself can address. 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] Re: [COMMITTERS] pgsql: pg_test_timing utility, to measure clock monotonicity and timing
On Wed, Mar 28, 2012 at 9:19 PM, Robert Haas robertmh...@gmail.com wrote: On Tue, Mar 27, 2012 at 10:10 PM, Fujii Masao masao.fu...@gmail.com wrote: On Wed, Mar 28, 2012 at 5:17 AM, Robert Haas rh...@postgresql.org wrote: pg_test_timing utility, to measure clock monotonicity and timing cost. When I compiled this, I got a compiler warning. Attached patch silences the warning. Unfortunately, that *produces* a warning on my machine. Normally, I think we handle this using INT64_FORMAT, but the fact that it's %10ld here and not just %lld makes that awkward. I guess we maybe need to insert some kludgy workaround here - write it into a separate buffer, and then blank-pad it, or something like that. This seems a simplest workaround. How about attached patch? Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center pgtesttiming_int64format.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] 9.2 commitfest closure (was Command Triggers, v16)
On Wed, Mar 28, 2012 at 09:45:29AM -0400, Robert Haas wrote: On Tue, Mar 27, 2012 at 1:36 PM, Robert Haas robertmh...@gmail.com wrote: Let's split the difference: how about we close it a week from this Friday. ?That would be April 6, 2012, ten days from today. Anybody, anybody? Can we try to get some agreement on this? +1 -- 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: pg_stat_statements normalisation without invasive changes to the parser (was: Next steps on pg_stat_statements normalisation)
On 28 March 2012 15:25, Tom Lane t...@sss.pgh.pa.us wrote: That's been an issue right along for cases such as EXPLAIN and EXECUTE, I believe. Possible, since I didn't have test coverage for either of those 2 commands. Perhaps the right thing is to consider such executor calls as nested statements --- that is, the ProcessUtility hook ought to bump the nesting depth too. That makes a lot of sense, but it might spoil things for the pg_stat_statements.track = 'top' + pg_stat_statements.track_utility = 'on' case. At the very least, it's a POLA violation, to the extent that if you were going to do this, you might mandate that nested statements be tracked along with utility statements (probably while defaulting to having both off, which would be a change). Since you've already removed the intoClause chunk, I'm not sure how far underway the review effort is - would you like me to produce a new revision, or is that unnecessary? -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and 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] triggers and inheritance tree
On Wed, Mar 28, 2012 at 8:29 AM, Robert Haas robertmh...@gmail.com wrote: I think the problem is that the UPDATE or DELETE can only fire once a matching row has been identified, so that OLD can be filled in appropriately. But in this case, the matching row gets found not in the parent table, but in one of its child tables. So any triggers on the child table would fire, but triggers on the parent table will not. ah! and of course that makes a lot of sense... how embarrasing! :( -- Jaime Casanova www.2ndQuadrant.com Professional PostgreSQL: Soporte 24x7 y capacitación -- 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: pg_stat_statements normalisation without invasive changes to the parser (was: Next steps on pg_stat_statements normalisation)
A couple other issues about this patch ... Is there any actual benefit in providing the pg_stat_statements.string_key GUC? It looks to me more like something that was thrown in because it was easy than because anybody would want it. I'd just as soon leave it out and avoid the incremental API complexity increase. (While on that subject, I see no documentation updates in the patch...) Also, I'm not terribly happy with the sticky entries hack. The way you had it set up with a 1e10 bias for a sticky entry was completely unreasonable IMO, because if the later pgss_store call never happens (which is quite possible if the statement contains an error detected during planning or execution), that entry is basically never going to age out, and will just uselessly consume a precious table slot for a long time. In the extreme, somebody could effectively disable query tracking by filling the hashtable with variants of SELECT 1/0. The best quick answer I can think of is to reduce USAGE_NON_EXEC_STICK to maybe 10 or so, but I wonder whether there's some less klugy way to get the result in the first place. I thought about keeping the canonicalized query string around locally in the backend rather than having the early pgss_store call at all, but am not sure it's worth the complexity of an additional local hashtable or some such to hold such pending entries. 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] Finer Extension dependencies
On Thu, Mar 22, 2012 at 2:08 PM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Alvaro Herrera alvhe...@commandprompt.com writes: get_available_versions_for_extension seems to contain a bunch of commented-out lines ... Damn. Sorry about that. Here's a cleaned-up version of the patch. I'm not completely convinced that the case has been made that this is a useful thing to have. It is true that systems like RPM have (and need) something like this, but that doesn't seem like a sufficient argument. The difference is that any set of RPMs which is designed to be used as a set has been packaged by a single project which (hopefully) has a consistent set of policies for how things are tagged and labeled and has done some integration testing to makes sure that every package which provides the wumpus feature actually is sufficient for the needs of all packages that depend on wumpus. On the other hand, PostgreSQL extensions are a free-for-all. Anybody can publish an extension on PGXN (or elsewhere), and they can stuff whatever junk they want in their control file - or more likely, fail to stuff anything in there at all, so that a package author who might want to depend on feature X may well find that not all the packages that provide feature X are tagged as providing it. If that turns out to be the case, are they going to (a) contact all of those package authors and convince them to update their control files or (b) punt? I'll bet on (b). Even if we suppose that the above is not a problem, it seems to me that there's a further difficulty. What exactly does it mean to provide a feature like smtp? Presumably, it means that you have a way to send mail. Fine. But, what exactly is that way? It presumably involves calling a function. It is very likely that if there are multiple mail-sending extension packages for PostgreSQL, they don't all create a function with exactly the same name and signature. Even if they did, the extension that is depending on this functionality has no way of knowing what schema that function is going to be in, unless all of those extensions are not-relocatable, which I think extension authors will be reluctant to do for entirely valid reasons. Now, the extension author can hack around all this by writing code (in PL/pgsql, for example) to search through the system catalogs and figure out which extension is providing the smtp feature and in what schema it's located; and then, having done that, they can even deduce the function name and signature, build a dynamically generated SQL query, and send mail. Woohoo! In practice, however, that sounds like a real pain in the neck. I would expect most people who were packaging extensions to handle a situation like this by forcing the user to provide the name of the function to be called, either via a control table or via a GUC. And once you've done that, it makes no sense to shove a feature dependency into the extension, because the user might very well just write an appropriate function themselves and tell the extension to call that. Theory aside, I am, like Alvaro, a bit skeptical of making extension features their own first-class objects. I think that part of the point of this mechanism in other package management systems is to allow a user to execute an RPM (say) transaction that drops an extension which provides feature X but makes up for it by installing, in the same transaction, a new extension that provides the same feature X. I suspect that this won't work with the design you have right now. In fact, I suspect it also won't work to install the new extension first and then drop the old one; I might be wrong, but I don't think our dependency mechanism has any support for depending on either A or B, which is really what is needed here. -- 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] triggers and inheritance tree
On Wed, Mar 28, 2012 at 10:46 AM, Jaime Casanova ja...@2ndquadrant.com wrote: On Wed, Mar 28, 2012 at 8:29 AM, Robert Haas robertmh...@gmail.com wrote: I think the problem is that the UPDATE or DELETE can only fire once a matching row has been identified, so that OLD can be filled in appropriately. But in this case, the matching row gets found not in the parent table, but in one of its child tables. So any triggers on the child table would fire, but triggers on the parent table will not. ah! and of course that makes a lot of sense... how embarrasing! :( If it's any consolation, when I initially looked at your example, I couldn't see what was wrong with it, either. After I ran it I figured it out. :-) -- 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] PATCH: pg_basebackup (missing exit on error)
On Wed, Mar 28, 2012 at 9:40 PM, Robert Haas robertmh...@gmail.com wrote: I think it would make sense to rearrange that so that we don't have two tests for ztarfile != NULL; do that test first, and then if it fails, do the strcmp after that. Makes sense. Also, if we're going to test the return value of fclose(), shouldn't we also be checking the return value of gzclose()? Yes, we should. Attached patch does the above two changes. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center missing_exit_on_error_in_pgbasebackup_v2.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] Re: [COMMITTERS] pgsql: pg_test_timing utility, to measure clock monotonicity and timing
Fujii Masao masao.fu...@gmail.com writes: This seems a simplest workaround. How about attached patch? I think you need to tweak that to get the number to be right-justified not left-justified. 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] Finer Extension dependencies
Robert Haas robertmh...@gmail.com writes: I'm not completely convinced that the case has been made that this is a useful thing to have. You're basically saying that the current lack of extension distribution is a good reason for not building the tools allowing to create said distribution. WTF? (PGXN uses the word distribution with yet another meaning, not to be mistaken for the kind of work done by debian or fedora teams) Even if we suppose that the above is not a problem, it seems to me that there's a further difficulty. What exactly does it mean to provide a feature like smtp? Presumably, it means that you have a Now you're saying that you want another feature built on top of this one, which is the ability to normalize features so that each provider is following up on the same standard as soon as they happen to provide the same feature. Then you say that having a features mechanism without the whole policy and standardisation and normalisation is not going to fly. WTF? In practice, however, that sounds like a real pain in the neck. I would expect most people who were packaging extensions to handle a situation like this by forcing the user to provide the name of the function to be called, either via a control table or via a GUC. And once you've done that, it makes no sense to shove a feature dependency into the extension, because the user might very well just write an appropriate function themselves and tell the extension to call that. I don't know what you're talking about here, all I can say is that is has nothing to do with what the patch is implementing. What's in the patch is a way to depend on known versions of an extension rather than the extension wholesale, whatever the version. Using feature dependency allow to avoid 2 particularly messy things: - imposing a version numbering scheme with a comparator - maintaining a separate feature matrix So instead of having to say foo version 1.2 is now doing buzz and having an extension depend on foo = 1.2, you can say that your extension depends on the buzz feature. That's about it. Theory aside, I am, like Alvaro, a bit skeptical of making extension features their own first-class objects. I think that part of the point of this mechanism in other package management systems is to allow a user to execute an RPM (say) transaction that drops an extension which provides feature X but makes up for it by installing, in the same transaction, a new extension that provides the same feature X. I suspect that this won't work with the design you have As far as I know, RPM and deb and other popular packaging systems are imposing mutually incompatible version number policies and tools to compare them, and then packagers have to manually care about translating a feature matrix into version number CHECK clauses. The require/provide facility is something more spread into Emacs Lisp and Common Lisp, if you really need to see it in action someplace else. But again, really, it's all about being able to have an extension depend on « the new hstore » or « the new pg_stat_statement ». That's it. About having a new catalog to host extension features, the problems I'm trying to solve with that are about extension upgrade. You can add new features at upgrade, you can also drop some. I don't know how to tell the user that extension X has to be removed to upgrade foo to 1.3 where it's not providing feature buzz anymore, without pg_extension_feature. right now. In fact, I suspect it also won't work to install the new extension first and then drop the old one; I might be wrong, but I don't think our dependency mechanism has any support for depending on either A or B, which is really what is needed here. We have ALTER EXTENSION foo UPDATE, you know. Regards, -- 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] Finer Extension dependencies
On Wed, Mar 28, 2012 at 11:28 AM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: In practice, however, that sounds like a real pain in the neck. I would expect most people who were packaging extensions to handle a situation like this by forcing the user to provide the name of the function to be called, either via a control table or via a GUC. And once you've done that, it makes no sense to shove a feature dependency into the extension, because the user might very well just write an appropriate function themselves and tell the extension to call that. I don't know what you're talking about here, all I can say is that is has nothing to do with what the patch is implementing. What's in the patch is a way to depend on known versions of an extension rather than the extension wholesale, whatever the version. Using feature dependency allow to avoid 2 particularly messy things: - imposing a version numbering scheme with a comparator - maintaining a separate feature matrix So instead of having to say foo version 1.2 is now doing buzz and having an extension depend on foo = 1.2, you can say that your extension depends on the buzz feature. That's about it. Based on this information, it seems that I've misinterpreted the purpose of the patch. Since extension features seem to live in a global namespace, I assumed that the purpose of the patch was to allow both extension A and extension B to provide feature F, and extension C to depend on F rather than A or B specifically. What I understand you to be saying here is that's not really what you're trying to accomplish. Instead, you're just concerned about allowing some but not all versions of package A to provide feature F, so that other extensions can depend on F to get the specific version of A that they need (and not, as I had assumed, so that they can get either A or B). Let me think more about that. Maybe I'm just easily confused here, or maybe there is something that should be changed in the code or docs; I'm not sure yet. On a more prosaic note, you seem to have made a mistake when generating the v5 diff. It includes reverts of a couple of unrelated, recent patches. WTF? WTF? On a further note, I have spent a heck of a lot more time reviewing other people's patches this CommitFest than you have, and I don't appreciate this. If you'd rather that I didn't spend time on this patch, I have plenty of other things to do with my time. -- 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] Feature proposal: list role members in psql
On Wed, Mar 28, 2012 at 01:55:06PM +0300, Anssi Kääriäinen wrote:
It seems there is no way to get role members using psql. \du and \dg
give you this role is a member of information, but no information
about who have been granted the role.
I would like to write a patch implementing this feature. Some questions:
- Is this wanted?
It is by at least by one person :)
- Should there be a new flag for this: \du[m+] where m list the
role memebers. Or could this be added to the '+' flag?
See below.
- Any pointers for patches implementing a similar addition to
psql? That would help me a lot, as I do not know the code base.
It's a pretty standard catalog lookup. Lemme see about a patch...
- Should the roles be fetched recursively, or should it be just
the direct role members? My opinion is that direct role members is
what is wanted.
The output would be something like:
Role name | Attributes | Member of| Role members
---+--++---
hot2_dba | Cannot login | {hot2_user,common_dba} | {akaariai}
- Anssi Kääriäinen
I'd say add first-level Role members to \du, all the way up the DAG (or
tree, if you're structured like that) for \du+
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
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] Finer Extension dependencies
Robert Haas robertmh...@gmail.com writes: accomplish. Instead, you're just concerned about allowing some but not all versions of package A to provide feature F, so that other extensions can depend on F to get the specific version of A that they need (and not, as I had assumed, so that they can get either A or B). Exactly. Let me think more about that. Maybe I'm just easily confused here, or maybe there is something that should be changed in the code or docs; I'm not sure yet. Agreed, there's certainly something to expand on here. On a more prosaic note, you seem to have made a mistake when generating the v5 diff. It includes reverts of a couple of unrelated, recent patches. Ouch. It seems to happen to me too often. I probably need to get the shallow clones setup where you have different directories hosting each a different git branch so that you don't need to checkout just to update your local master, etc. WTF? WTF? On a further note, I have spent a heck of a lot more time reviewing other people's patches this CommitFest than you have, and I don't appreciate this. If you'd rather that I didn't spend time on this patch, I have plenty of other things to do with my time. Sorry about that. I'm on a crazy schedule and too tired, and I wanted to be sure to attract your attention on a misunderstanding here. It's also not clear to me what level of language WTF really is, or “dude” to take another example. I'll be sure not to use that again when I aim at being polite yet dense. Regards, -- 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
[HACKERS] max_files_per_process ignored on Windows
At postmaster startup, we determine the maximum number of open files we can handle by trying to open a lot of file descriptors, up to max_files_per_process. This is done in set_max_safe_fds(), and the determined max_safe_fds value is inherited by child processes at fork(). However, with EXEC_BACKEND, ie. Windows, it's not inherited, so we always run with the initial conservative default of 32. An obvious fix would be to call set_max_safe_fds() in the child processes, although I wonder if that's too expensive. Another option is to pass down the value with save_restore_backend_variables(). Thoughts? Although this has apparently always been like this, no-one has complained, so I'm thinking that we shouldn't backport this. -- Heikki Linnakangas 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] Review of pg_archivecleanup -x option patch
On Wed, Mar 28, 2012 at 8:47 AM, Robert Haas robertmh...@gmail.com wrote: $ ./pg_archivecleanup -x bz2 /tmp 000100010058 Hmm, but I thought that the idea was that the extension was optional. Perhaps I'm missing something but I don't think the previous patch will complain about that either; or at least I don't see why the behavior should be any different. Can someone enlighten me on this point? mmm! you're right... it's not complaining either... i was sure it was... and i'm not sure i want to contor things for that... so, just forget my last mail about that... your refactor is just fine for me -- Jaime Casanova www.2ndQuadrant.com Professional PostgreSQL: Soporte 24x7 y capacitación -- 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] max_files_per_process ignored on Windows
On Wed, Mar 28, 2012 at 18:12, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: At postmaster startup, we determine the maximum number of open files we can handle by trying to open a lot of file descriptors, up to max_files_per_process. This is done in set_max_safe_fds(), and the determined max_safe_fds value is inherited by child processes at fork(). However, with EXEC_BACKEND, ie. Windows, it's not inherited, so we always run with the initial conservative default of 32. An obvious fix would be to call set_max_safe_fds() in the child processes, although I wonder if that's too expensive. Another option is to pass down the value with save_restore_backend_variables(). ISTM that passing down through save_restore_backend_variables() is a much better choice. Thoughts? Although this has apparently always been like this, no-one has complained, so I'm thinking that we shouldn't backport this. We should absolutely *not* backport this. It needs to go through some proper testing first, it might cause serious effects on some systems. In particular, it might have yet another round of effects on people who run with AV enabled... -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.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] Finer Extension dependencies
On Wed, Mar 28, 2012 at 12:11 PM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: On a more prosaic note, you seem to have made a mistake when generating the v5 diff. It includes reverts of a couple of unrelated, recent patches. Ouch. It seems to happen to me too often. I probably need to get the shallow clones setup where you have different directories hosting each a different git branch so that you don't need to checkout just to update your local master, etc. Could you possibly generate a new diff to save me the trouble of fixing the one you sent before? WTF? WTF? On a further note, I have spent a heck of a lot more time reviewing other people's patches this CommitFest than you have, and I don't appreciate this. If you'd rather that I didn't spend time on this patch, I have plenty of other things to do with my time. Sorry about that. I'm on a crazy schedule and too tired, and I wanted to be sure to attract your attention on a misunderstanding here. It's also not clear to me what level of language WTF really is, or “dude” to take another example. I'll be sure not to use that again when I aim at being polite yet dense. I don't seriously feel that WTF is unacceptable language for a public mailing list populated mostly by hackers; I try not to use it myself, but I'm not puritanical enough to get annoyed with other people for doing so. I was reacting to the overall tone rather than that specific expression. And I can sympathize with the crazy schedule and too tired thing; I have been there. -- 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] Re: pg_stat_statements normalisation without invasive changes to the parser (was: Next steps on pg_stat_statements normalisation)
On 28 March 2012 15:57, Tom Lane t...@sss.pgh.pa.us wrote: Is there any actual benefit in providing the pg_stat_statements.string_key GUC? It looks to me more like something that was thrown in because it was easy than because anybody would want it. I'd just as soon leave it out and avoid the incremental API complexity increase. (While on that subject, I see no documentation updates in the patch...) Personally, I don't care for it, and I'm sure most users wouldn't either, but I thought that someone somewhere might be relying on the existing behaviour. I will produce a doc-patch. It would have been premature to do so until quite recently. Also, I'm not terribly happy with the sticky entries hack. The way you had it set up with a 1e10 bias for a sticky entry was completely unreasonable IMO, because if the later pgss_store call never happens (which is quite possible if the statement contains an error detected during planning or execution), that entry is basically never going to age out, and will just uselessly consume a precious table slot for a long time. In the extreme, somebody could effectively disable query tracking by filling the hashtable with variants of SELECT 1/0. The best quick answer I can think of is to reduce USAGE_NON_EXEC_STICK to maybe 10 or so, but I wonder whether there's some less klugy way to get the result in the first place. I thought about keeping the canonicalized query string around locally in the backend rather than having the early pgss_store call at all, but am not sure it's worth the complexity of an additional local hashtable or some such to hold such pending entries. I was troubled by that too, and had considered various ways of at least polishing the kludge. Maybe a better approach would be to start with a usage of 1e10 (or something rather high, anyway), but apply a much more aggressive multiplier than USAGE_DECREASE_FACTOR for sticky entries only? That way, in earlier calls of entry_dealloc() the sticky entries, easily identifiable as having 0 calls, are almost impossible to evict, but after a relatively small number of calls they soon become more readily evictable. This seems better than simply having some much lower usage that is only a few times the value of USAGE_INIT. Let's suppose we set sticky entries to have a usage value of 10. If all other queries have more than 10 calls, which is not unlikely (under the current usage format, 1.0 usage = 1 call, at least until entry_dealloc() dampens usage) then when we entry_dealloc(), the sticky entry might as well have a usage of 1, and has no way of increasing its usage short of becoming a real entry. On the other hand, with the multiplier trick, how close the sticky entry is to eviction is, importantly, far more strongly influenced by the number of entry_dealloc() calls, which in turn is influenced by churn in the system, rather than being largely influenced by how the magic sticky usage value happens to compare to those usage values of some random set of real entries on some random database. If entries really are precious, then the sticky entry is freed much sooner. If not, then why not allow the sticky entry to stick around pending its execution/ promotion to a real entry? It would probably be pretty inexpensive to maintain what is currently the largest usage value in the hash table at entry_dealloc() time - that would likely be far more suitable than 1e10, and might even work well. We could perhaps cut that in half every entry_dealloc(). -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and 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] [COMMITTERS] pgsql: Remove dead assignment
On mån, 2012-03-26 at 15:53 -0400, Tom Lane wrote: Peter Eisentraut pete...@gmx.net writes: On mån, 2012-03-26 at 15:15 -0400, Tom Lane wrote: I also do not think it does anything for readability for this call of read_info() to be unexpectedly unlike all the others. I do not think that it is good code quality to assign something to a variable and then assign something different to a variable later in the same function. Well, that's a totally different issue, because if we had used a different variable for the other purpose, this assignment would still be dead and coverity would still be whinging about it, no? Let's look at this differently. If this code were written from scratch now, it might have turned out like this: Form_pg_sequence old_seq, seq; ... old_seq = read_info(elm, seq_rel, buf); ... seq = (Form_pg_sequence) GETSTRUCT(tuple); But that gets a complaint from gcc: sequence.c:248:19: error: variable ‘old_seq’ set but not used [-Werror=unused-but-set-variable] So when faced with this, what is the right fix? (Probably not assigning the useless return value to some other variable used for a different purpose.) The problem that I have with this change (and the similar ones you've made elsewhere) is really that it's only chance that the code isn't fetching anything from the result of read_info. What other changes are you referring to? I don't recall any similar ones and don't find any in the logs. -- 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 parallel pg_dump
On Sun, Mar 25, 2012 at 10:50 PM, Joachim Wieland j...@mcknight.de wrote:
On Fri, Mar 23, 2012 at 11:11 AM, Alvaro Herrera
alvhe...@commandprompt.com wrote:
Are you going to provide a rebased version?
Rebased version attached, this patch also includes Robert's earlier
suggestions.
I keep hoping someone who knows Windows is going to take a look at
this, but so far no luck. It could also really use some attention
from someone who has an actual really big database handy, to see how
successful it is in reducing the dump time. Without those things, I
can't see this getting committed. But in the meantime, a few fairly
minor comments based on reading the code.
I'm wondering if we really need this much complexity around shutting
down workers. I'm not sure I understand why we need both a hard and
a soft method of shutting them down. At least on non-Windows
systems, it seems like it would be entirely sufficient to just send a
SIGTERM when you want them to die. They don't even need to catch it;
they can just die. You could also set things up so that if the
connection to the parent process is closed, the worker exits. Then,
during normal shutdown, you don't need to kill them at all. The
master can simply exit, and the child processes will follow suit. The
checkAborting stuff all goes away.
The existing coding of on_exit_nicely is intention, and copied from
similar logic for on_shmem_exit in the backend. Is there really a
compelling reason to reverse the firing order of exit hooks?
On my system:
parallel.c: In function ‘WaitForTerminatingWorkers’:
parallel.c:275: warning: ‘slot’ may be used uninitialized in this function
make: *** [parallel.o] Error 1
Which actually looks like a semi-legitimate gripe.
+ if (numWorkers MAXIMUM_WAIT_OBJECTS)
+ {
+ fprintf(stderr, _(%s: invalid number of parallel
jobs\n), progname);
+ exit(1);
+ }
I think this error message could be more clear. How about maximum
number of parallel jobs is %d?
+void _SetupWorker(Archive *AHX, RestoreOptions *ropt) {}
Thankfully, this bit in pg_dumpall.c appears to be superfluous. I
hope this is just a holdover from an earlier version that we can lose.
- const char *modulename,
const char *fmt,...)
+ const char *modulename,
const char *fmt,...)
Useless hunk.
--
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
[HACKERS] pgxs and bison, flex
There are some extensions that build with pgxs that use bison and flex. Their makefiles are set up to use the variables BISON and FLEX that pgxs provides. Except that that depends on how PostgreSQL was built. A binary package that was built in a clean chroot would probably not have those variables set, because the programs were not present in the build process. There have been a number of bugs related to those extensions because of that. I propose that we apply the attached patch to make sure those variables are set to a usable default value in any case. diff --git i/src/makefiles/pgxs.mk w/src/makefiles/pgxs.mk index 7dc8742..318d5ef 100644 --- i/src/makefiles/pgxs.mk +++ w/src/makefiles/pgxs.mk @@ -64,6 +64,16 @@ include $(top_builddir)/src/Makefile.global top_srcdir = $(top_builddir) srcdir = . VPATH = + +# These might be set in Makefile.global, but if they were not found +# during the build of PostgreSQL, supply default values so that users +# of pgxs can use the variables. +ifeq ($(BISON),) +BISON = bison +endif +ifeq ($(FLEX),) +FLEX = flex +endif endif -- 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 parallel pg_dump
Excerpts from Robert Haas's message of mié mar 28 14:46:30 -0300 2012: I keep hoping someone who knows Windows is going to take a look at this, but so far no luck. It could also really use some attention from someone who has an actual really big database handy, to see how successful it is in reducing the dump time. Without those things, I can't see this getting committed. But in the meantime, a few fairly minor comments based on reading the code. My main comment about the current patch is that it looks like it's touching pg_restore parallel code by moving some stuff into parallel.c. If that's really the case and its voluminous, maybe this patch would shrink a bit if we could do the code moving in a first patch. That would be mostly mechanical. Then the interesting stuff would apply on top of that. That would make review easier. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 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] patch for parallel pg_dump
On Wed, Mar 28, 2012 at 2:20 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Robert Haas's message of mié mar 28 14:46:30 -0300 2012: I keep hoping someone who knows Windows is going to take a look at this, but so far no luck. It could also really use some attention from someone who has an actual really big database handy, to see how successful it is in reducing the dump time. Without those things, I can't see this getting committed. But in the meantime, a few fairly minor comments based on reading the code. My main comment about the current patch is that it looks like it's touching pg_restore parallel code by moving some stuff into parallel.c. If that's really the case and its voluminous, maybe this patch would shrink a bit if we could do the code moving in a first patch. That would be mostly mechanical. Then the interesting stuff would apply on top of that. That would make review easier. +1. -- 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] patch for parallel pg_dump
On 03/28/2012 02:27 PM, Robert Haas wrote: On Wed, Mar 28, 2012 at 2:20 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Robert Haas's message of mié mar 28 14:46:30 -0300 2012: I keep hoping someone who knows Windows is going to take a look at this, but so far no luck. It could also really use some attention from someone who has an actual really big database handy, to see how successful it is in reducing the dump time. Without those things, I can't see this getting committed. But in the meantime, a few fairly minor comments based on reading the code. My main comment about the current patch is that it looks like it's touching pg_restore parallel code by moving some stuff into parallel.c. If that's really the case and its voluminous, maybe this patch would shrink a bit if we could do the code moving in a first patch. That would be mostly mechanical. Then the interesting stuff would apply on top of that. That would make review easier. +1. +1 also. FYI I am just starting some test runs on Windows. I also have a reasonably sized db (98 gb) on my SL6 server which should be perfect for testing this (I just partitioned its two main tables), and will try to get some timing runs. 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] Command Triggers patch v18
Robert Haas robertmh...@gmail.com writes: Further thought: I think maybe we shouldn't use keywords at all for this, and instead use descriptive strings like post-parse or pre-execution or command-start, because I bet in the end we're going to end up with a bunch of them (create-schema-subcommand-start? alter-table-subcommand-start?), and it would be nice not to hassle with the grammar every time we want to add a new one. To make this work with the grammar, we could either (1) enforce that each is exactly one word or (2) require them to be quoted or (3) require those that are not a single word to be quoted. I tend think #2 might be the best option in this case, but... What about : create command trigger foo before prepare alter table … create command trigger foo before start of alter table … create command trigger foo before execute alter table … create command trigger foo before end of alter table … create command trigger foo when prepare alter table … create command trigger foo when start alter table … create command trigger foo when execute of alter table … create command trigger foo when end of alter table … create command trigger foo preceding alter table … create command trigger foo preceding alter table … deferred create command trigger foo preceding alter table … immediate create command trigger foo following parser of alter table … create command trigger foo preceding execute of alter table … create command trigger foo following alter table … create command trigger foo before alter table nowait … I'm not sure how many hooks we can really export here, but I guess we have enough existing keywords to describe when they get to run (parser, mapping, lock, check, begin, analyze, access, disable, not exists, do, end, escape, extract, fetch, following, search…) I've also had another general thought about safety: we need to make sure that we're only firing triggers from places where it's safe for user code to perform arbitrary actions. In particular, we have to assume that any triggers we invoke will AcceptInvalidationMessages() and CommandCounterIncrement(); and we probably can't do it at all (or at least not without some additional safeguard) in places where the code does CheckTableNotInUse() up through the point where it's once again safe to access the table, since the trigger might try. We also I've been trying to do that already. need to think about re-entrancy: that is, in theory, the trigger might execute any other DDL command - e.g. it might drop the table that we've been asked to rename. I suspect that some of the current BEFORE That's why I implemented ALTER COMMAND TRIGGER ... SET DISABLE in the first place, so that you could run the same command from the trigger itself without infinite recursion. trigger stuff might fall down on that last one, since the existing code not-unreasonably expects that once it's locked the table, the catalog entries can't go away. Maybe it all happens to work out OK, but I don't think we can count on that. I didn't think of DROP TABLE in a command table running BEFORE ALTER TABLE, say. That looks evil. It could be documented as yet another way to shoot yourself in the foot though? Regards, -- 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] Standbys, txid_current_snapshot, wraparound
On Fri, Mar 23, 2012 at 08:52:40AM +, Simon Riggs wrote: Master pg_controldata - OK txid_current_snapshot() - OK Standby pg_controldata - OK txid_current_snapshot() - lower value On Skytools list is report about master with slaves, but the lower value appears on master too: http://lists.pgfoundry.org/pipermail/skytools-users/2012-March/001601.html Cc'd original reporter too. -- marko -- 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] poll: CHECK TRIGGER?
Hello 2012/3/28 Heikki Linnakangas heikki.linnakan...@enterprisedb.com: Ok, seems that the API issue is settled, so I'm now looking at the code actually doing the checking. My first impression is that this is a lot of code. Can we simplify it? I am afraid so there are not a big space for simplification :( Since this is deeply integrated into the PL/pgSQL interpreter, I was expecting that this would run through the normal interpreter, in a special mode that skips all the actual execution of queries, and shortcuts all loops and other control statements so that all code is executed only once. That would mean sprinkling some if (check_only) code into the normal exec_* functions. I'm not sure how invasive that would be, but it's worth considering. I think you would be able to more easily catch more errors that way, and the check code would stay better in sync with the execution code. This can mess current code - it can works, but some important fragments can be less readable after. Almost all eval routines should supports fake mode, and it can be little bit slower and less readable. Another thought is that check_stmt() and all its subroutines are very similar to the plpgsql_dumptree() code. Would it make sense to merge those? You could have an output mode, in addition to the xml and plain-text formats, that would just dump the whole tree like plpgsql_dumptree() does. yes, it is possible - first implementation was via walker, and it was reused for dump. Current code is more readable, but there is not reuse. I am able to redesign code to this direction. In prepare_expr(), you use a subtransaction to catch any ERRORs that happen during parsing the expression. That's a good idea, and I think many of the check_* functions could be greatly simplified by adopting a similar approach. Just ereport() any errors you find, and catch them at the appropriate level, appending the error to the output string. Your current approach of returning true/false depending on whether there was any errors seems tedious. This is not possible, when we would to enable fatal_errors = false checking. I can do subtransaction in prepare_expr, because it is the most deep level, but I cannot to use it elsewhere, because I cannot handle exception and continue with other parts of statement. If you create a function with an invalid body (ie. set check_function_bodies=off; create function ... $$ bogus $$;) , plpgsql_check_function() still throws an error. It's understandable that it cannot do in-depth analysis if the function cannot be parsed, but I would expect the syntax error to be returned as a return value like other errors that it complains about, not thrown as a hard ERROR. That would make it more useful to bulk-check all functions in a database with something like select plpgsql_check_function(oid) from pg_class. As it is, the checking stops at the first invalid function with an error. it is good idea PS. I think plpgsql_check_function() belongs in pl_handler.c can be - why not. Regards Pavel -- Heikki Linnakangas 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] patch for parallel pg_dump
On 03/28/2012 03:17 PM, Andrew Dunstan wrote: On 03/28/2012 02:27 PM, Robert Haas wrote: On Wed, Mar 28, 2012 at 2:20 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Robert Haas's message of mié mar 28 14:46:30 -0300 2012: I keep hoping someone who knows Windows is going to take a look at this, but so far no luck. It could also really use some attention from someone who has an actual really big database handy, to see how successful it is in reducing the dump time. Without those things, I can't see this getting committed. But in the meantime, a few fairly minor comments based on reading the code. My main comment about the current patch is that it looks like it's touching pg_restore parallel code by moving some stuff into parallel.c. If that's really the case and its voluminous, maybe this patch would shrink a bit if we could do the code moving in a first patch. That would be mostly mechanical. Then the interesting stuff would apply on top of that. That would make review easier. +1. +1 also. FYI I am just starting some test runs on Windows. I also have a reasonably sized db (98 gb) on my SL6 server which should be perfect for testing this (I just partitioned its two main tables), and will try to get some timing runs. First hurdle: It doesn't build under Windows/mingw-w64: x86_64-w64-mingw32-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 -I./src/include/port/win32 -DEXEC_BACKEND -I/c/prog/mingwdep/include -I../../../src/include/port/win32 -c -o parallel.o parallel.c parallel.c:40:12: error: static declaration of 'pgpipe' follows non-static declaration ../../../src/include/port.h:268:12: note: previous declaration of 'pgpipe' was here parallel.c:41:12: error: static declaration of 'piperead' follows non-static declaration ../../../src/include/port.h:269:12: note: previous declaration of 'piperead' was here parallel.c: In function 'ParallelBackupStart': parallel.c:455:9: warning: passing argument 3 of '_beginthreadex' from incompatible pointer type c:\mingw64\bin\../lib/gcc/x86_64-w64-mingw32/4.5.3/../../../../x86_64-w64-mingw32/include/process.h:31:29: note: expected 'unsigned int (*)(void *)' but argument is of type 'unsigned int (*)(struct WorkerInfo *)' make[3]: *** [parallel.o] Error 1 make[3]: Leaving directory `/home/andrew/bf/root/HEAD/pgsql/src/bin/pg_dump' make[2]: *** [all-pg_dump-recurse] Error 2 make[2]: Leaving directory `/home/andrew/bf/root/HEAD/pgsql/src/bin' make[1]: *** [all-bin-recurse] Error 2 make[1]: Leaving directory `/home/andrew/bf/root/HEAD/pgsql/src' make: *** [all-src-recurse] Error 2 I'll have a look at that in a little while. cheers andrew 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] Standbys, txid_current_snapshot, wraparound
On Wed, Mar 28, 2012 at 9:48 PM, Marko Kreen mark...@gmail.com wrote: On Fri, Mar 23, 2012 at 08:52:40AM +, Simon Riggs wrote: Master pg_controldata - OK txid_current_snapshot() - OK Standby pg_controldata - OK txid_current_snapshot() - lower value On Skytools list is report about master with slaves, but the lower value appears on master too: http://lists.pgfoundry.org/pipermail/skytools-users/2012-March/001601.html Cc'd original reporter too. Thanks. Am looking. -- 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] Finer Extension dependencies
On Wed, Mar 28, 2012 at 3:09 PM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Robert Haas robertmh...@gmail.com writes: Could you possibly generate a new diff to save me the trouble of fixing the one you sent before? Please find it attached, it looks better now, and I rebased it against master for good measure (no conflicts). Comments: I tested this out and it seems to work as designed. I think the lack of pg_upgrade support is a must-fix before commit. It seems to me that's a non-trivial problem, as I think you're going to need to invent a way to jam the feature list into the stub extension, either something like binary_upgrade.add_feature_to_extension() or new syntax like ALTER EXTENSION .. ADD FEATURE. Don't we need some psql support for listing the features provided by an installed extension? And an available extension? get_extension_feature_oids seems to be misnamed, since it returns only one OID, not more than one, and it also returns the feature OID. At a minimum, I think you need to delete the s from the function name; possibly it should be renamed to lookup_extension_feature or somesuch. List *requires; /* names of prerequisite extensions */ + List *provides; /* names of provided features */ Comment in requires line of above hunk should be updated to say prerequisite features. @@ -4652,4 +4652,3 @@ DESCR(SP-GiST support for suffix tree over text); #define PROARGMODE_TABLE 't' #endif /* PG_PROC_H */ - Useless hunk. +errmsg(parameter \%s\ must be a list of extension names, This error, which relates to the parsing of the provides list, say extension features, and the existing code for requires needs to be updated to say that as well. +errmsg(extension feature \%s\ already exists [%u], + feature, featoid))); That [%u] at the end there does not conform to our message style guidelines, and I think the rest of the message could be improved a bit as well. I think maybe it'd be appropriate to work the name of the other extension into the message, maybe something like: extension %s provides feature %s, but existing extension %s already provides this feature +extern char * get_extension_feature_name(Oid featoid); Extra space. + if (strcmp(curreq,pcontrol-name) != 0) Missing space (after the comma). OCLASS_EXTENSION_FEATURE should probable be added to the penultimate switch case in ATExecAlterColumnType. Gotta run, there may be more but I'm out of time to stare at this for the moment. -- 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] Standbys, txid_current_snapshot, wraparound
On Wed, Mar 28, 2012 at 10:24 PM, Simon Riggs si...@2ndquadrant.com wrote: On Wed, Mar 28, 2012 at 9:48 PM, Marko Kreen mark...@gmail.com wrote: On Fri, Mar 23, 2012 at 08:52:40AM +, Simon Riggs wrote: Master pg_controldata - OK txid_current_snapshot() - OK Standby pg_controldata - OK txid_current_snapshot() - lower value On Skytools list is report about master with slaves, but the lower value appears on master too: http://lists.pgfoundry.org/pipermail/skytools-users/2012-March/001601.html Cc'd original reporter too. Thanks. Am looking. I can't see how this could happen on the master at all. On the standby, it can happen if we skip restartpoints for about a couple of billion xids. Which would be a problem. More on this tomorrow. -- 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] Re: pg_stat_statements normalisation without invasive changes to the parser (was: Next steps on pg_stat_statements normalisation)
Peter Geoghegan pe...@2ndquadrant.com writes: On 28 March 2012 15:57, Tom Lane t...@sss.pgh.pa.us wrote: Is there any actual benefit in providing the pg_stat_statements.string_key GUC? It looks to me more like something that was thrown in because it was easy than because anybody would want it. I'd just as soon leave it out and avoid the incremental API complexity increase. (While on that subject, I see no documentation updates in the patch...) Personally, I don't care for it, and I'm sure most users wouldn't either, but I thought that someone somewhere might be relying on the existing behaviour. Hearing no squawks, I will remove it from the committed patch; one less thing to document. Easy enough to put back later, if someone makes a case for it. Also, I'm not terribly happy with the sticky entries hack. I was troubled by that too, and had considered various ways of at least polishing the kludge. Maybe a better approach would be to start with a usage of 1e10 (or something rather high, anyway), but apply a much more aggressive multiplier than USAGE_DECREASE_FACTOR for sticky entries only? That way, in earlier calls of entry_dealloc() the sticky entries, easily identifiable as having 0 calls, are almost impossible to evict, but after a relatively small number of calls they soon become more readily evictable. I did some simple experiments with the regression tests. Now, those tests are by far a worst case for this sort of thing, since (a) they probably generate many more unique queries than a typical production application, and (b) they almost certainly provoke many more errors and hence more dead sticky entries than a typical production app. Nonetheless, the results look pretty bad. Using various values of USAGE_NON_EXEC_STICK, the numbers of useful and dead entries in the hash table after completing one round of regression tests was: STICK live entriesdead sticky entries 10.0780 190 5.0 858 112 4.0 874 96 3.0 911 62 2.0 918 43 I did not bother measuring 1e10 ;-). It's clear that sticky entries are forcing useful entries out of the table in this scenario. I think wasting more than about 10% of the table in this way is not acceptable. I'm planning to commit the patch with a USAGE_NON_EXEC_STICK value of 3.0, which is the largest value that stays below 10% wastage. We can twiddle that logic later, so if you want to experiment with an alternate decay rule, feel free. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: pg_stat_statements normalisation without invasive changes to the parser (was: Next steps on pg_stat_statements normalisation)
On 29 March 2012 00:14, Tom Lane t...@sss.pgh.pa.us wrote: I'm planning to commit the patch with a USAGE_NON_EXEC_STICK value of 3.0, which is the largest value that stays below 10% wastage. We can twiddle that logic later, so if you want to experiment with an alternate decay rule, feel free. I think I may well end up doing so when I get a chance. This seems like the kind of problem that will be solved only when we get some practical experience (i.e. use the tool on something closer to a production system than the regression tests). doc-patch is attached. I'm not sure if I got the balance right - it may be on the verbose side. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services pg_stat_statements_norm_docs.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] patch for parallel pg_dump
On Wed, Mar 28, 2012 at 5:19 PM, Andrew Dunstan and...@dunslane.net wrote: First hurdle: It doesn't build under Windows/mingw-w64: parallel.c:40:12: error: static declaration of 'pgpipe' follows non-static declaration Strange, I'm not seeing this but I'm building with VC2005. What happens is that you're pulling in the pgpipe.h header. I have moved these functions as static functions into pg_dump since you voted for removing them from the other location (because as it turned out, nobody else is currently using them). Joachim -- 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] ECPG FETCH readahead
On Sat, Mar 24, 2012 at 10:49:07AM +0100, Boszormenyi Zoltan wrote: Sorry for the delay, I had been busy with other tasks and I rewrote this code to better cope with unknown result size, scrollable cursors and negative cursor positions. I think all points raised by Noah is addressed: per-cursor readahead window size, extensive comments, documentation and not enabling result set size discovery. The new comments are a nice improvement; thanks. The problem with WHERE CURRENT OF is solved by a little more grammar and ecpglib code, which effectively does a MOVE ABSOLUTE N before executing the DML with WHERE CURRENT OF clause. No patching of the backend. This way, the new ECPG caching code is compatible with older servers but obviously reduces the efficiency of caching. Good plan. diff -dcrpN postgresql.orig/doc/src/sgml/ecpg.sgml postgresql/doc/src/sgml/ecpg.sgml *** postgresql.orig/doc/src/sgml/ecpg.sgml2012-03-12 09:24:31.699560098 +0100 --- postgresql/doc/src/sgml/ecpg.sgml 2012-03-24 10:15:00.538924601 +0100 *** EXEC SQL COMMIT; *** 454,459 --- 454,479 details. /para + para +ECPG may use cursor readahead to improve performance of programs +that use single-row FETCH statements. Option literal-R number/literal +option for ECPG modifies the default for all cursors from literalNO READAHEAD/literal This sentence duplicates the word option. +to literalREADAHEAD number/literal. Explicit literalREADAHEAD number/literal or +literalNO READAHEAD/literal turns cursor readahead on (with literalnumber/literal +as the size of the readahead window) or off for the specified cursor, +respectively. For cursors using a non-0 readahead window size is 256 rows, The number 256 is no longer present in your implementation. +the window size may be modified by setting the literalECPGFETCHSZ/literal +environment variable to a different value. I had in mind that DECLARE statements adorned with READAHEAD syntax would always behave precisely as written, independent of ECPGFETCHSZ or ecpg -R. Unadorned DECLARE statements would use ECPGFETCHSZ if set, then the value passed to ecpg -R if provided, and finally a value of 1 (no readahead) in the absence of both ECPGFETCHSZ and ecpg -R. Did you do it differently for a particular reason? I don't particularly object to what you've implemented, but I'd be curious to hear your thinking. + /para + + para +commandUPDATE/command or commandDELETE/command with the +literalWHERE CURRENT OF/literal clause, cursor readahead may or may not +actually improve performance, as commandMOVE/command statement has to be +sent to the backend before the DML statement to ensure correct cursor position +in the backend. + /para This sentence seems to be missing a word near its beginning. *** DECLARE replaceable class=PARAMETERc *** 6639,6649 /listitem /varlistentry /variablelist para ! For the meaning of the cursor options, ! see xref linkend=sql-declare. /para /refsect1 refsect1 --- 6669,6728 /listitem /varlistentry /variablelist +/refsect1 + +refsect1 + titleCursor options/title para ! For the meaning of other cursor options, see xref linkend=sql-declare. /para + + variablelist + varlistentry + termliteralREADAHEAD number/literal/term + termliteralNO READAHEAD/literal/term +listitem + para + literalREADAHEAD number/literal makes the ECPG preprocessor and + runtime library use a client-side cursor accounting and data readahead + during commandFETCH/command. This improves performance for programs + that use single-row commandFETCH/command statements. + /para + + para + literalNO READAHEAD/literal disables data readahead in case + parameter-R number/parameter is used for compiling the file. + /para +/listitem + /varlistentry One of the new sections about readahead should somehow reference the hazard around volatile functions. + + varlistentry + termliteralOPEN RETURNS LAST ROW POSITION/literal/term + termliteralOPEN RETURNS 0 FOR LAST ROW POSITION/literal/term +listitem + para + When the cursor is opened, it's possible to discover the size of the result set + using commandMOVE ALL/command which traverses the result set and move + the cursor back to the beginning using commandMOVE ABSOLUTE 0/command. + The size of the result set is returned in sqlca.sqlerrd[2]. + /para + + para + This slows down opening the cursor from the application point of view + but may also have side effects. If the cursor query
Re: [HACKERS] patch for parallel pg_dump
On 03/28/2012 08:28 PM, Joachim Wieland wrote: On Wed, Mar 28, 2012 at 5:19 PM, Andrew Dunstanand...@dunslane.net wrote: First hurdle: It doesn't build under Windows/mingw-w64: parallel.c:40:12: error: static declaration of 'pgpipe' follows non-static declaration Strange, I'm not seeing this but I'm building with VC2005. What happens is that you're pulling in the pgpipe.h header. I have moved these functions as static functions into pg_dump since you voted for removing them from the other location (because as it turned out, nobody else is currently using them). But your patch hasn't got rid of them, and so it's declared twice. There is no pgpipe.h, BTW, it's declared in port.h. If VC2005 doesn't complain about the double declaration then that's a bug in the compiler, IMNSHO. Doesn't it even issue a warning? I no longer use VC2005 for anything, BTW, I use VC2008 or later. Anyway, ISTM the best thing is just for us to get rid of pgpipe without further ado. I'll try to get a patch together for that. cheers andrew Joachim -- 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: pg_stat_statements normalisation without invasive changes to the parser (was: Next steps on pg_stat_statements normalisation)
Peter Geoghegan pe...@2ndquadrant.com writes: doc-patch is attached. I'm not sure if I got the balance right - it may be on the verbose side. Thanks. I've committed the patch along with the docs, after rather heavy editorialization. There remain some loose ends that should be worked on but didn't seem like commit-blockers: 1. What to do with EXPLAIN, SELECT INTO, etc. We had talked about tweaking the behavior of statement nesting and some other possibilities. I think clearly this could use improvement but I'm not sure just how. (Note: I left out the part of your docs patch that attempted to explain the current behavior, since I think we should fix it not document it.) 2. Whether and how to adjust the aging-out of sticky entries. This seems like a research project, but the code impact should be quite localized. BTW, I eventually concluded that the parameterization testing we were worried about before was a red herring. As committed, the patch tries to store a normalized string if it found any deletable constants, full stop. This seems to me to be correct behavior because the presence of constants is exactly what makes the string normalizable, and such constants *will* be ignored in the hash calculation no matter whether there are other parameters or not. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for parallel pg_dump
On Thu, Mar 29, 2012 at 2:46 AM, Andrew Dunstan and...@dunslane.net wrote: But your patch hasn't got rid of them, and so it's declared twice. There is no pgpipe.h, BTW, it's declared in port.h. If VC2005 doesn't complain about the double declaration then that's a bug in the compiler, IMNSHO. Doesn't it even issue a warning? I no longer use VC2005 for anything, BTW, I use VC2008 or later. I agree, the compiler should have found it, but no, I don't even get a warning. I just verified it and when I add a #error right after the prototypes in port.h then it hits the #error on every file, so it definitely sees both prototypes and doesn't complain... cl.exe is running with /W3. Anyway, ISTM the best thing is just for us to get rid of pgpipe without further ado. I'll try to get a patch together for that. Thanks. -- 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 parallel pg_dump
On Wed, Mar 28, 2012 at 1:46 PM, Robert Haas robertmh...@gmail.com wrote: I'm wondering if we really need this much complexity around shutting down workers. I'm not sure I understand why we need both a hard and a soft method of shutting them down. At least on non-Windows systems, it seems like it would be entirely sufficient to just send a SIGTERM when you want them to die. They don't even need to catch it; they can just die. At least on my Linux test system, even if all pg_dump processes are gone, the server happily continues sending data. When I strace an individual backend process, I see a lot of Broken pipe writes, but that doesn't stop it from just writing out the whole table to a closed file descriptor. This is a 9.0-latest server. --- SIGPIPE (Broken pipe) @ 0 (0) --- read(13, \220\370\0\0\240\240r\266\3\0\4\0\264\1\320\1\0 \4 \0\0\0\0\270\237\220\0h\237\230\0..., 8192) = 8192 read(13, \220\370\0\0\350\300r\266\3\0\4\0\264\1\320\1\0 \4 \0\0\0\0\260\237\230\0h\237\220\0..., 8192) = 8192 sendto(7, d\0\0\0Acpp\t15.0\t124524\taut..., 8192, 0, NULL, 0) = -1 EPIPE (Broken pipe) --- SIGPIPE (Broken pipe) @ 0 (0) --- read(13, \220\370\0\\341r\266\3\0\5\0\260\1\340\1\0 \4 \0\0\0\0\270\237\220\0p\237\220\0..., 8192) = 8192 sendto(7, d\0\0\0Dcpp\t15.0\t1245672000\taut..., 8192, 0, NULL, 0) = -1 EPIPE (Broken pipe) --- SIGPIPE (Broken pipe) @ 0 (0) --- read(13, unfinished ... I guess that https://commitfest.postgresql.org/action/patch_view?id=663 would take care of that in the future, but without it (i.e anything = 9.2) it's quite annoying if you want to Ctrl-C a pg_dump and then have to manually hunt down and kill all the backend processes. I tested the above with immediately returning from DisconnectDatabase() in pg_backup_db.c on Linux. The important thing is that it calls PQcancel() on it's connection before terminating. On Windows, several pages indicate that you can only cleanly terminate a thread from within the thread, e.g. the last paragraph on http://msdn.microsoft.com/en-us/library/windows/desktop/ms686724%28v=vs.85%29.aspx The patch is basically doing what this page is recommending. I'll try your proposal about terminating in the child when the connection is closed, that sounds reasonable and I don't see an immediate problem with that. The existing coding of on_exit_nicely is intention, and copied from similar logic for on_shmem_exit in the backend. Is there really a compelling reason to reverse the firing order of exit hooks? No, reversing the order was not intended. I rewrote it to a for loop because the current implementation modifies a global variable and so on Windows only one thread would call the exit hook. I'll add all your other suggestions to the next version of my patch. Thanks! Joachim -- 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] max_files_per_process ignored on Windows
Magnus Hagander mag...@hagander.net writes: On Wed, Mar 28, 2012 at 18:12, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: An obvious fix would be to call set_max_safe_fds() in the child processes, although I wonder if that's too expensive. Another option is to pass down the value with save_restore_backend_variables(). ISTM that passing down through save_restore_backend_variables() is a much better choice. +1. Having multiple children running set_max_safe_fds() concurrently would be a seriously bad idea. 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] pgxs and bison, flex
Peter Eisentraut pete...@gmx.net writes: There are some extensions that build with pgxs that use bison and flex. Their makefiles are set up to use the variables BISON and FLEX that pgxs provides. Except that that depends on how PostgreSQL was built. A binary package that was built in a clean chroot would probably not have those variables set, because the programs were not present in the build process. There have been a number of bugs related to those extensions because of that. I propose that we apply the attached patch to make sure those variables are set to a usable default value in any case. Won't this break usages such as in contrib/cube? cubeparse.c: cubeparse.y ifdef BISON $(BISON) $(BISONFLAGS) -o $@ $ else @$(missing) bison $ $@ endif IMO, people building distribution packages in clean chroots are best advised to include bison and flex even if they're not strictly necessary. Otherwise the build could fall over unexpectedly and unnecessarily, depending on file timestamps and other phase-of-the-moon issues. If the package maker has adopted that elementary bit of self-defense, the whole thing is a non problem. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: pg_test_timing utility, to measure clock monotonicity and timing
On Thu, Mar 29, 2012 at 12:15 AM, Tom Lane t...@sss.pgh.pa.us wrote: Fujii Masao masao.fu...@gmail.com writes: This seems a simplest workaround. How about attached patch? I think you need to tweak that to get the number to be right-justified not left-justified. Unless I'm missing something, I did that because the patch uses %10s not %-10s. No? Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- 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: pg_stat_statements normalisation without invasive changes to the parser (was: Next steps on pg_stat_statements normalisation)
BTW, I forgot to mention that I did experiment with your python-based test script for pg_stat_statements, but decided not to commit it. There are just too many external dependencies for my taste: 1. python 2. psycopg2 3. dellstore2 test database That coupled with the apparent impossibility of running the script without manual preconfiguration makes it look not terribly useful. Also, as of the committed patch there are several individual tests that fail or need adjustment: The SELECT INTO tests all fail, but we know the reason why (the testbed isn't expecting them to result in creating separate entries for the utility statement and the underlying plannable SELECT). verify_statement_equivalency(select a.orderid from orders a join orders b on a.orderid = b.orderid, select b.orderid from orders a join orders b on a.orderid = b.orderid, conn) These are not equivalent statements, or at least would not be if the join condition were anything else than what it is, so the fact that the original coding failed to distinguish the targetlist entries is a bug. The test # temporary column name within recursive CTEs doesn't differentiate fails, not because of the change of column name, but because of the change of CTE name. This is a consequence of my having used the CTE name here: case RTE_CTE: /* * Depending on the CTE name here isn't ideal, but it's the * only info we have to identify the referenced WITH item. */ APP_JUMB_STRING(rte-ctename); APP_JUMB(rte-ctelevelsup); break; We could avoid the name dependency by omitting ctename from the jumble but I think that cure is worse than the disease. Anyway, not too important, but I just thought I'd document this in case you were wondering about the discrepancies. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: pg_test_timing utility, to measure clock monotonicity and timing
Fujii Masao masao.fu...@gmail.com writes: On Thu, Mar 29, 2012 at 12:15 AM, Tom Lane t...@sss.pgh.pa.us wrote: I think you need to tweak that to get the number to be right-justified not left-justified. Unless I'm missing something, I did that because the patch uses %10s not %-10s. No? Oh, you're right, I was misremembering which direction that went. Sorry for the noise. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for parallel pg_dump
On 03/28/2012 09:12 PM, Joachim Wieland wrote: On Thu, Mar 29, 2012 at 2:46 AM, Andrew Dunstanand...@dunslane.net wrote: But your patch hasn't got rid of them, and so it's declared twice. There is no pgpipe.h, BTW, it's declared in port.h. If VC2005 doesn't complain about the double declaration then that's a bug in the compiler, IMNSHO. Doesn't it even issue a warning? I no longer use VC2005 for anything, BTW, I use VC2008 or later. I agree, the compiler should have found it, but no, I don't even get a warning. I just verified it and when I add a #error right after the prototypes in port.h then it hits the #error on every file, so it definitely sees both prototypes and doesn't complain... cl.exe is running with /W3. src/bin/pg_dump/pg_backup_archiver.c Anyway, ISTM the best thing is just for us to get rid of pgpipe without further ado. I'll try to get a patch together for that. Thanks. OK, I have committed this. Following that, you need to add a couple of macros for pipewrite to parallel.c. There's also a missing cast in the call to beginthreadex(). I'll continue testing tomorrow. 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] incorrect handling of the timeout in pg_receivexlog
On Tue, Feb 28, 2012 at 6:08 PM, Fujii Masao masao.fu...@gmail.com wrote: On Wed, Feb 8, 2012 at 1:33 AM, Magnus Hagander mag...@hagander.net wrote: Will it break using pg_basebackup 9.2 on a 9.1 server, though? that would also be very useful in the scenario of the central server... No unless I'm missing something. Because pg_basebackup doesn't use any message which is defined in walprotocol.h if -x stream option is not specified. No, this is not right at all :( Changing TimestampTz fields in 9.2 would break that use case. If we support that use case, pg_basebackup 9.2 must know which an integer or a double is used for TimestampTz in 9.1 server. Otherwise pg_basebackup cannot process a WAL data message proporly. But unfortunately there is no way for pg_basebackup 9.2 to know that... 9.1 has no API to report the actual datatype of its TimestampTz field. One idea to support that use case is to add new command-line option into pg_basebackup, which specifies the datatype of TimestampTz field. You can use one pg_basebackup 9.2 executable on 9.1 server whether --disable-integer-datetimes is specified or not. But I'm not really sure if it's worth doing this, because ISTM that it's rare to build a server and a client with the different choice about TimestampTz datatype. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
