Re: [PATCHES] Free WAL caches on switching segments
On Mon, 2006-02-13 at 23:33 -0500, Tom Lane wrote: > Bruce Momjian writes: > > Yes, your vote counts very much. What if I apply the patch, but mark > > the posix_advise() call in a NOT_USED macro block, so it will be ready > > for people to test, but will not be used until we are sure. > > Sounds like a recipe for ensuring it never will be tested. What's > needed here is some actual tests, not preparation... Without discussing this particular patch, IMHO we need a clear checklist of items that are required before a patch is accepted onto the patches awaiting application list. We seem to have had a few patches rejected on grounds that could have been picked up by other people, not just Tom and Bruce. This wastes everybody's time because the patch writer might have fixed whatever was wrong with it a while back if the patch had been rejected earlier. It will also eventually bring the process into disrepute if one accepts patches onto the queue and then another raises major objections that could easily have been rectified earlier. So let's agree a checklist beforehand. That way patch submitters can be told to resubmit much earlier by other list watchers, with less wasted time (elapsed and from core folk) and perhaps a gentler experience for first-time submitters. Control can and should still lie with committers. Suggested checklist: 1. has patch been discussed previously? Y/N - if Y, give direct link to archive of message, and/or bug# - if N discuss on appropriate list, or expect to be rejected 2. patch target: cvstip or stated branch(es) 3. patch application location: root directory only 4. patch format: diff -c only 5. confirm licence is BSD: Y/N 6. port specific: Y/N, if Y list ports 7. confirm passes make check (on listed ports) 8. provide implementation overview, preferably in code comments 9. if it is a performance patch, provide confirming test results. It is OK to post patches without these, though the patch will not be applied until *somebody* has tested the patches and found a valuable performance effect directly attributable to the patch. 10. If it is a new feature patch, confirm that it has been tested for all desired scenarios. If it has not, this should be clearly stated as a request for a particular kind of test to be performed. Note that the patch will go no further until that test has been performed. 11. if it is a new feature patch, does it break any existing defaults? Explain why this is *required* or patch will be rejected. New feature patches should be accompanied by doc patches also. 12. Even if you pass all of the above, the patch may still be rejected for other technical reasons. You should be prepared to listen to comments received and perform any agreed rework. Even if you have received positive comments from some community members, others may spot problems with your approach, coding style or many other issues. 13. Successful patches will be notified to you by email and you will be credited for that work in the next set of release notes. I would also suggest that we have two patch queues: - patches awaiting performance testing - patches awaiting application (current one) That way anybody wanting to test new performance add-ons can do so and reply to the list with confirmation that the patch can now be added to the second (main) list of patches. Of course, this suggestion will be immediately rejected because it wasn't discussed on -hackers ;-) Best Regards, Simon Riggs ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] Free WAL caches on switching segments
Mark Kirkwood <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >> Sounds like a recipe for ensuring it never will be tested. What's >> needed here is some actual tests, not preparation... > Does the OP have a test scenario that those of us with appropriate OS's > could try? Come to think of it, what are the appropriate OS's? (I see > NetBSD mentioned so I suppose all the *BSDs, but what others?). The test run by the OP was just pgbench, which is probably not the greatest scenario for showing the benefits of this patch, but at least it's neutral ground. You need a situation in which the kernel is under memory stress, else early free of disk cache buffers isn't going to make any difference whatever --- so choose a pgbench scale factor that makes the database noticeably larger than the test machine's RAM. Other than that, follow the usual guidelines for producing trustworthy pgbench numbers: number of clients smaller than scale factor, number of transactions per client at least 1000 or so (to eliminate startup transients), repeat test a couple times to make sure numbers are reproducible. regards, tom lane ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] Free WAL caches on switching segments
On Tue, 2006-02-14 at 12:54 +, Simon Riggs wrote: > On Mon, 2006-02-13 at 23:33 -0500, Tom Lane wrote: > > Bruce Momjian writes: > > > Yes, your vote counts very much. What if I apply the patch, but mark > > > the posix_advise() call in a NOT_USED macro block, so it will be ready > > > for people to test, but will not be used until we are sure. > > > > Sounds like a recipe for ensuring it never will be tested. What's > > needed here is some actual tests, not preparation... > > Without discussing this particular patch, IMHO we need a clear checklist > of items that are required before a patch is accepted onto the patches > awaiting application list. This was supposed to be a serious suggestion, so apologies if this came across stronger than it was meant. The onus is of course upon the patch submitter to improve their game, but there seems only benefit in setting out the (simpler) rules of the game to show people what is unacceptable, even before they submit. Best Regards, Simon Riggs ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] Free WAL caches on switching segments
Simon Riggs <[EMAIL PROTECTED]> writes: > This was supposed to be a serious suggestion, so apologies if this came > across stronger than it was meant. If you want to have a serious discussion about it, you need to start a thread on pghackers under a more suitable subject line. The people reading this thread are going to be a relatively small group. regards, tom lane ---(end of broadcast)--- TIP 6: explain analyze is your friend
[PATCHES] add additional options to CREATE TABLE ... AS
This patch adds most of the options available for regular CREATE TABLE syntax to the CREATE TABLE x AS SELECT ... and AS EXECUTE ... Specifically this allows specification of on commit behavior for temp tables and tablespaces for regular tables to these two statements. Additionally with/without oids is now available for the EXECUTE variant. Currently you still cannot specify inheritance attributes with these commands, but this seems like a more complicated task. Kris Jurka? GNUmakefile ? config.log ? config.status ? log ? contrib/spi/.deps ? src/Makefile.global ? src/backend/postgres ? src/backend/access/common/.deps ? src/backend/access/gist/.deps ? src/backend/access/hash/.deps ? src/backend/access/heap/.deps ? src/backend/access/index/.deps ? src/backend/access/nbtree/.deps ? src/backend/access/transam/.deps ? src/backend/bootstrap/.deps ? src/backend/catalog/.deps ? src/backend/catalog/postgres.bki ? src/backend/catalog/postgres.description ? src/backend/catalog/postgres.shdescription ? src/backend/commands/.deps ? src/backend/executor/.deps ? src/backend/lib/.deps ? src/backend/libpq/.deps ? src/backend/main/.deps ? src/backend/nodes/.deps ? src/backend/optimizer/geqo/.deps ? src/backend/optimizer/path/.deps ? src/backend/optimizer/plan/.deps ? src/backend/optimizer/prep/.deps ? src/backend/optimizer/util/.deps ? src/backend/parser/.deps ? src/backend/port/.deps ? src/backend/postmaster/.deps ? src/backend/regex/.deps ? src/backend/rewrite/.deps ? src/backend/storage/buffer/.deps ? src/backend/storage/file/.deps ? src/backend/storage/freespace/.deps ? src/backend/storage/ipc/.deps ? src/backend/storage/large_object/.deps ? src/backend/storage/lmgr/.deps ? src/backend/storage/page/.deps ? src/backend/storage/smgr/.deps ? src/backend/tcop/.deps ? src/backend/utils/.deps ? src/backend/utils/adt/.deps ? src/backend/utils/cache/.deps ? src/backend/utils/error/.deps ? src/backend/utils/fmgr/.deps ? src/backend/utils/hash/.deps ? src/backend/utils/init/.deps ? src/backend/utils/mb/.deps ? src/backend/utils/mb/conversion_procs/conversion_create.sql ? src/backend/utils/mb/conversion_procs/ascii_and_mic/.deps ? src/backend/utils/mb/conversion_procs/ascii_and_mic/libascii_and_mic.so.0.0 ? src/backend/utils/mb/conversion_procs/cyrillic_and_mic/.deps ? src/backend/utils/mb/conversion_procs/cyrillic_and_mic/libcyrillic_and_mic.so.0.0 ? src/backend/utils/mb/conversion_procs/euc_cn_and_mic/.deps ? src/backend/utils/mb/conversion_procs/euc_cn_and_mic/libeuc_cn_and_mic.so.0.0 ? src/backend/utils/mb/conversion_procs/euc_jp_and_sjis/.deps ? src/backend/utils/mb/conversion_procs/euc_jp_and_sjis/libeuc_jp_and_sjis.so.0.0 ? src/backend/utils/mb/conversion_procs/euc_kr_and_mic/.deps ? src/backend/utils/mb/conversion_procs/euc_kr_and_mic/libeuc_kr_and_mic.so.0.0 ? src/backend/utils/mb/conversion_procs/euc_tw_and_big5/.deps ? src/backend/utils/mb/conversion_procs/euc_tw_and_big5/libeuc_tw_and_big5.so.0.0 ? src/backend/utils/mb/conversion_procs/latin2_and_win1250/.deps ? src/backend/utils/mb/conversion_procs/latin2_and_win1250/liblatin2_and_win1250.so.0.0 ? src/backend/utils/mb/conversion_procs/latin_and_mic/.deps ? src/backend/utils/mb/conversion_procs/latin_and_mic/liblatin_and_mic.so.0.0 ? src/backend/utils/mb/conversion_procs/utf8_and_ascii/.deps ? src/backend/utils/mb/conversion_procs/utf8_and_ascii/libutf8_and_ascii.so.0.0 ? src/backend/utils/mb/conversion_procs/utf8_and_big5/.deps ? src/backend/utils/mb/conversion_procs/utf8_and_big5/libutf8_and_big5.so.0.0 ? src/backend/utils/mb/conversion_procs/utf8_and_cyrillic/.deps ? src/backend/utils/mb/conversion_procs/utf8_and_cyrillic/libutf8_and_cyrillic.so.0.0 ? src/backend/utils/mb/conversion_procs/utf8_and_euc_cn/.deps ? src/backend/utils/mb/conversion_procs/utf8_and_euc_cn/libutf8_and_euc_cn.so.0.0 ? src/backend/utils/mb/conversion_procs/utf8_and_euc_jp/.deps ? src/backend/utils/mb/conversion_procs/utf8_and_euc_jp/libutf8_and_euc_jp.so.0.0 ? src/backend/utils/mb/conversion_procs/utf8_and_euc_kr/.deps ? src/backend/utils/mb/conversion_procs/utf8_and_euc_kr/libutf8_and_euc_kr.so.0.0 ? src/backend/utils/mb/conversion_procs/utf8_and_euc_tw/.deps ? src/backend/utils/mb/conversion_procs/utf8_and_euc_tw/libutf8_and_euc_tw.so.0.0 ? src/backend/utils/mb/conversion_procs/utf8_and_gb18030/.deps ? src/backend/utils/mb/conversion_procs/utf8_and_gb18030/libutf8_and_gb18030.so.0.0 ? src/backend/utils/mb/conversion_procs/utf8_and_gbk/.deps ? src/backend/utils/mb/conversion_procs/utf8_and_gbk/libutf8_and_gbk.so.0.0 ? src/backend/utils/mb/conversion_procs/utf8_and_iso8859/.deps ? src/backend/utils/mb/conversion_procs/utf8_and_iso8859/libutf8_and_iso8859.so.0.0 ? src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/.deps ? src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/libutf8_and_iso8859_1.so.0.0 ? src/backend/utils/mb/conversion_procs/utf8_and_johab/.deps ? src/backend/utils/mb/conversion_procs/utf8_and_johab/libutf8_and_johab.so.0.0 ? src/backend/utils/m
Re: [PATCHES] add additional options to CREATE TABLE ... AS
On Tue, 2006-02-14 at 14:32 -0500, Kris Jurka wrote:
> This patch adds most of the options available for regular CREATE TABLE
> syntax to the CREATE TABLE x AS SELECT ... and AS EXECUTE ...
> Specifically this allows specification of on commit behavior for temp
> tables and tablespaces for regular tables to these two statements.
The implementation is pretty ugly -- it clutters ExecuteStmt and Query
with fields that really do not belong there. Per previous discussion, I
think it would be better to refactor the CREATE TABLE AS implementation
to be essentially a CREATE TABLE followed by a INSERT ... SELECT.
(That's not necessarily a reason to reject the patch, but the patch does
increase the benefit of performing that refactoring.)
A few cosmetic comments:
typedef struct ExecuteStmt
{
NodeTag type;
char *name;
RangeVar *into;
ContainsOidsintocontainsoids;
boolintohasoids;
OnCommitAction intooncommit;
char *intotablespacename;
List *params;
} ExecuteStmt;
I think we ought to use either camel-case or underscore characters to
separate words.
parser/analyze.c, circa 1822:
if (stmt->intoTableSpaceName)
qry->intoTableSpaceName = pstrdup(stmt->intoTableSpaceName);
else
qry->intoTableSpaceName = NULL;
You can omit the "else", as makeNode() zeroes all the fields of the new
node. (You could argue that leaving the assignment is more readable, but
I personally don't think so: this behavior of makeNode() is used in a
several places in the backend.)
-Neil
---(end of broadcast)---
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match
Re: [PATCHES] add additional options to CREATE TABLE ... AS
Neil Conway <[EMAIL PROTECTED]> writes: > The implementation is pretty ugly -- it clutters ExecuteStmt and Query > with fields that really do not belong there. Per previous discussion, I > think it would be better to refactor the CREATE TABLE AS implementation > to be essentially a CREATE TABLE followed by a INSERT ... SELECT. I kinda wonder why bother at all. I don't see any good reason why people shouldn't issue two statements. >> if (stmt->intoTableSpaceName) >> qry->intoTableSpaceName = pstrdup(stmt->intoTableSpaceName); >> else >> qry->intoTableSpaceName = NULL; > You can omit the "else", as makeNode() zeroes all the fields of the new > node. For that matter, why not just qry->intoTableSpaceName = stmt->intoTableSpaceName; There's no need for the string-copy operation here, is there? regards, tom lane ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] add additional options to CREATE TABLE ... AS
On Tue, 14 Feb 2006, Kris Jurka wrote:
This patch adds most of the options available for regular CREATE TABLE syntax
to the CREATE TABLE x AS SELECT ... and AS EXECUTE ...
Here's the doc changes for this.
Kris JurkaIndex: doc/src/sgml/ref/create_table_as.sgml
===
RCS file: /projects/cvsroot/pgsql/doc/src/sgml/ref/create_table_as.sgml,v
retrieving revision 1.31
diff -c -r1.31 create_table_as.sgml
*** doc/src/sgml/ref/create_table_as.sgml 1 Nov 2005 21:09:50 -
1.31
--- doc/src/sgml/ref/create_table_as.sgml 14 Feb 2006 21:07:27 -
***
*** 21,27
CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } ] TABLE
table_name
! [ (column_name [, ...] ) ] [ [ WITH | WITHOUT
] OIDS ]
AS query
--- 21,30
CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } ] TABLE
table_name
! [ (column_name [, ...] ) ]
! [ WITH OIDS | WITHOUT OIDS ]
! [ ON COMMIT { PRESERVE ROWS | DELETE ROWS | DROP } ]
! [ TABLESPACE tablespace ]
AS query
***
*** 114,119
--- 117,180
+ ON COMMIT
+
+
+ The behavior of temporary tables at the end of a transaction
+ block can be controlled using ON COMMIT.
+ The three options are:
+
+
+
+ PRESERVE ROWS
+
+
+ No special action is taken at the ends of transactions.
+ This is the default behavior.
+
+
+
+
+
+ DELETE ROWS
+
+
+ All rows in the temporary table will be deleted at the
+ end of each transaction block. Essentially, an automatic
+is done at each commit.
+
+
+
+
+
+ DROP
+
+
+ The temporary table will be dropped at the end of the current
+ transaction block.
+
+
+
+
+
+
+
+
+
+ TABLESPACE tablespace
+
+
+ The tablespace is the name
+ of the tablespace in which the new table is to be created.
+ If not specified,
+is used, or the database's
+ default tablespace if default_tablespace is an empty
+ string.
+
+
+
+
+
query
***
*** 170,175
--- 231,250
SELECT * FROM films WHERE date_prod >= '2002-01-01';
+
+
+Create a new temporary table that will be dropped at commit
+films_recent with oids consisting of only
+recent entries from the table films using a
+prepared statement:
+
+
+ PREPARE recentfilms(date) AS
+ SELECT * FROM films WHERE date_prod > $1;
+ CREATE TEMP TABLE films_recent WITH OIDS ON COMMIT DROP AS
+ EXECUTE recentfilms('2002-01-01');
+
+
***
*** 190,202
- The standard defines an ON COMMIT clause;
- this is not currently implemented by PostgreSQL.
-
-
-
-
-
The standard defines a WITH [ NO ] DATA clause;
this is not currently implemented by PostgreSQL.
The behavior provided by PostgreSQL is equivalent
--- 265,270
***
*** 219,224
--- 287,300
for details.
+
+
+
+ The PostgreSQL concept of tablespaces is not
+ part of the standard. Hence, the clause TABLESPACE
+ is an extension.
+
+
---(end of broadcast)---
TIP 6: explain analyze is your friend
Re: [PATCHES] Free WAL caches on switching segments
Tom Lane wrote: Mark Kirkwood <[EMAIL PROTECTED]> writes: Tom Lane wrote: Sounds like a recipe for ensuring it never will be tested. What's needed here is some actual tests, not preparation... Does the OP have a test scenario that those of us with appropriate OS's could try? Come to think of it, what are the appropriate OS's? (I see NetBSD mentioned so I suppose all the *BSDs, but what others?). The test run by the OP was just pgbench, Ah - right, missed that sorry. which is probably not the greatest scenario for showing the benefits of this patch, but at least it's neutral ground. You need a situation in which the kernel is under memory stress, else early free of disk cache buffers isn't going to make any difference whatever --- so choose a pgbench scale factor that makes the database noticeably larger than the test machine's RAM. Other than that, follow the usual guidelines for producing trustworthy pgbench numbers: number of clients smaller than scale factor, number of transactions per client at least 1000 or so (to eliminate startup transients), repeat test a couple times to make sure numbers are reproducible. Thinking about this, presumably any write intensive, multi-user benchmark would seem to be suitable, so would something like OSDL's DBT-2 actually be better to try? Cheers Mark (P.s - academic in my case, unless I try out the latest NetBSD or Linux on one of my FreeBSD boxes) ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] Free WAL caches on switching segments
Mark Kirkwood <[EMAIL PROTECTED]> writes: > Thinking about this, presumably any write intensive, multi-user > benchmark would seem to be suitable, so would something like OSDL's > DBT-2 actually be better to try? I'm certainly not wedded to pgbench, give it a try. BTW, I forgot to mention that it would be useful to try different wal_sync_methods along with this. The reason why it seems unlikely the patch is useful on Linux is that the sync methods that use O_DIRECT probably dominate using the patch anyway. There may or may not be a similar dependence on sync method on other kernels ... regards, tom lane ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] add additional options to CREATE TABLE ... AS
On Tue, 14 Feb 2006, Tom Lane wrote: I kinda wonder why bother at all. I don't see any good reason why people shouldn't issue two statements. Well if you don't know what the resulting columns are going to be that could be difficult. There are a number of reasons why this patch is an improvement. 1) People have requested this feature: http://archives.postgresql.org/pgsql-bugs/2005-11/msg00163.php http://archives.postgresql.org/pgsql-bugs/2004-03/msg00186.php 2) The SQL spec requires ON COMMIT for CREATE TEMP TABLE AS SELECT. 3) The unification of EXECUTE and SELECT options actually simplifies the grammar by removing the WithOidsAs production hack. Kris Jurka ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
