Re: [PATCHES] Bulk Insert tuning
On Tue, 2008-02-26 at 21:36 +, Simon Riggs wrote:
On Tue, 2008-02-26 at 15:12 -0500, Tom Lane wrote:
Simon Riggs [EMAIL PROTECTED] writes:
Following patch implements a simple mechanism to keep a buffer pinned
while we are bulk loading.
This will fail to clean up nicely after a subtransaction abort, no?
Yes, will fix.
Additional line in AbortSubTransaction handles this.
(For that matter I don't think it's right even for a top-level abort.)
And I'm pretty sure it will trash your table entirely if someone
inserts into another relation while a bulk insert is happening.
(Not at all impossible, think of triggers for instance.)
The pinned buffer is separate from the preferred block for each
relation; BulkInsertBuffer isn't used for determining the block to
insert into. If you try to insert into a block that differs from the
pinned one it unpins it and re-pins the new one. So it is always safe
with respect to the data in the table.
It can run into recursive bulk insert ops but that just destroys the
performance advantage, its not actually dangerous.
I'm about to start refactoring code as suggested, so wanted to drop off
another version to allow everybody to examine the safety/not of this
approach. (So this patch is WIP)
--
Simon Riggs
2ndQuadrant http://www.2ndQuadrant.com
PostgreSQL UK 2008 Conference: http://www.postgresql.org.uk
Index: src/backend/access/heap/heapam.c
===
RCS file: /home/sriggs/pg/REPOSITORY/pgsql/src/backend/access/heap/heapam.c,v
retrieving revision 1.251
diff -c -r1.251 heapam.c
*** src/backend/access/heap/heapam.c 8 Mar 2008 21:57:59 - 1.251
--- src/backend/access/heap/heapam.c 20 Mar 2008 13:38:53 -
***
*** 1744,1749
--- 1744,1764
}
}
+ /*
+ * Begin/End Bulk Inserts
+ *
+ */
+ void
+ heap_begin_bulk_insert(void)
+ {
+ ReleaseBulkInsertBufferIfAny();
+ }
+
+ void
+ heap_end_bulk_insert(void)
+ {
+ ReleaseBulkInsertBufferIfAny();
+ }
/*
* heap_insert - insert tuple into a heap
***
*** 1771,1781
*/
Oid
heap_insert(Relation relation, HeapTuple tup, CommandId cid,
! bool use_wal, bool use_fsm)
{
TransactionId xid = GetCurrentTransactionId();
HeapTuple heaptup;
Buffer buffer;
if (relation-rd_rel-relhasoids)
{
--- 1786,1797
*/
Oid
heap_insert(Relation relation, HeapTuple tup, CommandId cid,
! bool use_wal, bool use_fsm, bool bulk_insert_request)
{
TransactionId xid = GetCurrentTransactionId();
HeapTuple heaptup;
Buffer buffer;
+ bool bulk_insert = bulk_insert_request !relation-rd_istemp;
if (relation-rd_rel-relhasoids)
{
***
*** 1828,1836
else
heaptup = tup;
! /* Find buffer to insert this tuple into */
! buffer = RelationGetBufferForTuple(relation, heaptup-t_len,
! InvalidBuffer, use_fsm);
/* NO EREPORT(ERROR) from here till changes are logged */
START_CRIT_SECTION();
--- 1844,1861
else
heaptup = tup;
! /*
! * Find buffer to insert this tuple into
! */
! if (bulk_insert)
! {
! buffer = RelationGetBufferForTuple(relation, heaptup-t_len,
! GetBulkInsertBuffer(), use_fsm, true);
! SetBulkInsertBuffer(buffer);
! }
! else
! buffer = RelationGetBufferForTuple(relation, heaptup-t_len,
! InvalidBuffer, use_fsm, false);
/* NO EREPORT(ERROR) from here till changes are logged */
START_CRIT_SECTION();
***
*** 1909,1915
END_CRIT_SECTION();
! UnlockReleaseBuffer(buffer);
/*
* If tuple is cachable, mark it for invalidation from the caches in case
--- 1934,1946
END_CRIT_SECTION();
! /*
! * Keep buffer pinned if we are in bulk insert mode
! */
! if (bulk_insert)
! LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
! else
! UnlockReleaseBuffer(buffer);
/*
* If tuple is cachable, mark it for invalidation from the caches in case
***
*** 1946,1952
Oid
simple_heap_insert(Relation relation, HeapTuple tup)
{
! return heap_insert(relation, tup, GetCurrentCommandId(true), true, true);
}
/*
--- 1977,1983
Oid
simple_heap_insert(Relation relation, HeapTuple tup)
{
! return heap_insert(relation, tup, GetCurrentCommandId(true), true, true, false);
}
/*
***
*** 2569,2575
{
/* Assume there's no chance to put heaptup on same page. */
newbuf = RelationGetBufferForTuple(relation, heaptup-t_len,
! buffer, true);
}
else
{
--- 2600,2606
{
/* Assume there's no chance to put heaptup on same page. */
newbuf = RelationGetBufferForTuple(relation, heaptup-t_len,
! buffer, true, false);
}
else
{
***
*** 2586,2592
*/
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
newbuf = RelationGetBufferForTuple(relation, heaptup-t_len,
!
Re: [PATCHES] Moving snapshot code around
On Tue, 2008-03-18 at 16:19 -0300, Alvaro Herrera wrote: I'm playing with the snapshot code to create a new module to stash used snapshots and refcount them. Sounds good. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com PostgreSQL UK 2008 Conference: http://www.postgresql.org.uk -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [GENERAL] Empty arrays with ARRAY[]
Brendan Jurd [EMAIL PROTECTED] writes:
A quick recap: I submitted a patch for empty ARRAY[] syntax back in
November, and as far as I can see it never made it to the patches
list. Gregory suggested a different way of approaching the problem
(quoted below), but nobody commented further about how it might be
made to work.
I'd like to RFC again on Gregory's idea, and if that doesn't bear any
fruit I'd like to submit the patch as-is for review.
Greg's idea is basically to invent array-of-UNKNOWN as a genuine
datatype, which as I stated way back when seems fairly dangerous
to me. UNKNOWN is already a pretty slippery animal, and I don't
know what cast paths we might open up by doing that. I think
the require-a-cast solution is a lot less likely to result in
unforeseen side-effects.
Whereas my patch requires you to write
a text[]: =array[]::text[];
... which seems pretty stupid.
In practice you'd write
DECLARE
a text[] := '{}';
which is even shorter, so I don't find this convincing.
regards, tom lane
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] pg_dump --no-tablespaces patch
Gavin M. Roy [EMAIL PROTECTED] writes: This is the patch I proposed on hackers to make pg_dump optionally ignore tablespaces. The patch is against 8.2.4. If I should be applying it to CVS head or what not, please let me know (along with any thoughts, concerns or issues). Applied with minor fixes and addition of documentation. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [GENERAL] Empty arrays with ARRAY[]
Brendan Jurd [EMAIL PROTECTED] writes:
As discussed on -hackers, this patch allows the construction of an
empty array if an explicit cast to an array type is given (as in,
ARRAY[]::int[]).
Applied with minor fixes; mostly, ensuring that the cast action would
propagate down to sub-arrays, as in
regression=# select array[[1],[2.2]]::int[];
array
---
{{1},{2}}
(1 row)
I was interested to realize that this fix validates the decision to
pass down the type information on-the-fly during transformExpr recursion.
It would have been a lot more painful to do it if we'd taken the A_Const
approach.
I didn't do anything about removing A_Const's typename field, but I'm
thinking that would be a good cleanup patch.
regards, tom lane
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Show INHERIT in \du
Brendan Jurd [EMAIL PROTECTED] writes: I've done up a patch per Tom's idea of combining the binary role attributes into a single column. I started to look at committing this and realized that there's a very nasty problem: our current approach to localizing the strings won't work. See this patch for background: http://archives.postgresql.org/pgsql-committers/2007-12/msg00187.php The code is now set up so that it can pass an entire field value through gettext(), but if gettext recognizes the strings foo and bar that doesn't mean it will do anything useful with foo\nbar, which is what this patch would require. I suspect that to solve this in a non-kluge fashion we'd need to make \du pull over the plain boolean and integer values, then build a new PGresult data structure on its own. Ugh. (Actually, without any support from libpq for building PGresults, it's hard to imagine doing that in a way that wouldn't be a kluge itself.) Or we could go back to the drawing board on what the output ought to look like. Thoughts? regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Show INHERIT in \du
Brendan Jurd escribió:
I've done up a patch per Tom's idea of combining the binary role
attributes into a single column.
Thanks -- this is nice. I even went to apply it, but found a problem:
gettext localizes the NULL string to the localization header :-( For
example:
alvherre=# \du
Liste des rôles
Nom du rôle | Attributes |
Membre de
-+-+---
alvherre| Superuser | {}
: Create role
: Create DB
asd | No inherit | {}
: No login
bar | Project-Id-Version: psql| {}
: Report-Msgid-Bugs-To:
: POT-Creation-Date: 2007-12-17 11:14-0400
: PO-Revision-Date: 2007-12-18 10:44+0100
: Last-Translator: Guillaume Lelarge [EMAIL PROTECTED]
: Language-Team: [EMAIL PROTECTED]
: MIME-Version: 1.0
: Content-Type: text/plain; charset=ISO-8859-15
: Content-Transfer-Encoding: 8bit
: X-Generator: KBabel 1.11.4
:
foo | No login|
{asd}
(4 lignes)
Starting psql with LC_ALL=C shows the truth:
alvherre=# \du
List of roles
Role name | Attributes | Member of
---+-+---
alvherre | Superuser | {}
: Create role
: Create DB
asd | No inherit | {}
: No login
bar | | {}
foo | No login| {asd}
(4 rows)
I'm not sure how to fix this. Thoughts?
--
Alvaro Herrerahttp://www.advogato.org/person/alvherre
Management by consensus: I have decided; you concede.
(Leonard Liu)
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Show INHERIT in \du
Alvaro Herrera [EMAIL PROTECTED] writes: gettext localizes the NULL string to the localization header :-( For example: Oooh, that's even different from the one I thought of :-(. Yeah, we've got a problem here. We could fix that particular issue by changing print.c so that it doesn't attempt to localize a zero-length string; but that won't help localizing multiple strings that have been concatenated. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix pgstatindex using for large indexes
Tatsuhito Kasahara wrote: Tatsuhito Kasahara wrote: I fix the patch. Oops, I forgot to attach the patch for pgstattuple.sql. I send it again. Hmm, this followup patch is wrong though -- the SQL definition is still using BIGINT where it should be using double. And the other changes to use BIGINT where the original values were int4 seem unnecessary. One thing I'm not clear about is the change from %d to %u to represent int4 values. Since the SQL datatype is signed, this can't really work, now, can it? -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix pgstatindex using for large indexes
Tatsuhito Kasahara [EMAIL PROTECTED] writes: Tom Lane wrote: Most places where we've dealt with this before, we use double, which is guaranteed to be available whereas uint64 is not ... Oh I see. I fix the patch. # I changed max_avail and free_space to double. I took a closer look at this, and noticed that you were still redefining the output parameters of the function as BIGINT: *** *** 33,48 -- pgstatindex -- CREATE OR REPLACE FUNCTION pgstatindex(IN relname text, ! OUT version int4, ! OUT tree_level int4, ! OUT index_size int4, ! OUT root_block_no int4, ! OUT internal_pages int4, ! OUT leaf_pages int4, ! OUT empty_pages int4, ! OUT deleted_pages int4, ! OUT avg_leaf_density float8, ! OUT leaf_fragmentation float8) AS 'MODULE_PATHNAME', 'pgstatindex' LANGUAGE C STRICT; --- 33,48 -- pgstatindex -- CREATE OR REPLACE FUNCTION pgstatindex(IN relname text, ! OUT version INT, ! OUT tree_level INT, ! OUT index_size BIGINT, ! OUT root_block_no INT, ! OUT internal_pages BIGINT, ! OUT leaf_pages BIGINT, ! OUT empty_pages BIGINT, ! OUT deleted_pages BIGINT, ! OUT avg_leaf_density FLOAT8, ! OUT leaf_fragmentation FLOAT8) AS 'MODULE_PATHNAME', 'pgstatindex' LANGUAGE C STRICT; This is inconsistent --- if int64 isn't actually available, it's not likely to work very well. It seems to me that we should either change the output parameters to float8, or stick with the original version of the patch and just accept that it will give overflowed answers on machines without working int64. Given that there is no problem until you get into multi-terabyte indexes, which are hardly likely to be getting pushed around on ancient systems, it's hard to argue that there's really any case against using bigint. Also I now see that pgstattuple() is using bigint for numbers that are really much more likely to overflow a broken int64 than pgstatindex() is, so the argument for float would require us to change both functions. In short, I'm willing to drop my opposition to the original form of the patch. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [GENERAL] Empty arrays with ARRAY[]
On 21/03/2008, Tom Lane [EMAIL PROTECTED] wrote: Brendan Jurd [EMAIL PROTECTED] writes: As discussed on -hackers, this patch allows the construction of an empty array if an explicit cast to an array type is given (as in, ARRAY[]::int[]). Applied with minor fixes; mostly, ensuring that the cast action would propagate down to sub-arrays, as in Great, thanks Tom. I was interested to realize that this fix validates the decision to pass down the type information on-the-fly during transformExpr recursion. It would have been a lot more painful to do it if we'd taken the A_Const approach. Indeed. I didn't do anything about removing A_Const's typename field, but I'm thinking that would be a good cleanup patch. I'd be happy to take this on. My day job is pretty busy at the moment but I should be able to submit something in a week or so. Cheers, BJ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix pgstatindex using for large indexes
Alvaro Herrera [EMAIL PROTECTED] writes: Hmm, this followup patch is wrong though -- the SQL definition is still using BIGINT where it should be using double. And the other changes to use BIGINT where the original values were int4 seem unnecessary. I'm on this one now ... regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix pgstatindex using for large indexes
I wrote: In short, I'm willing to drop my opposition to the original form of the patch. Original version applied with some minor tweaks (notably, pg_relpages had the same problem). regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Show INHERIT in \du
On 21/03/2008, Tom Lane [EMAIL PROTECTED] wrote: The code is now set up so that it can pass an entire field value through gettext(), but if gettext recognizes the strings foo and bar that doesn't mean it will do anything useful with foo\nbar, which is what this patch would require. Ouch! I suspect that to solve this in a non-kluge fashion we'd need to make \du pull over the plain boolean and integer values, then build a new PGresult data structure on its own. Ugh. (Actually, without any support from libpq for building PGresults, it's hard to imagine doing that in a way that wouldn't be a kluge itself.) Or we could go back to the drawing board on what the output ought to look like. We can't just build the output table by hand like describeOneTableDetails does? Admittedly it's kludgy, but it's not an unprecedented kludge. -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Show INHERIT in \du
Brendan Jurd [EMAIL PROTECTED] writes: On 21/03/2008, Tom Lane [EMAIL PROTECTED] wrote: The code is now set up so that it can pass an entire field value through gettext(), but if gettext recognizes the strings foo and bar that doesn't mean it will do anything useful with foo\nbar, which is what this patch would require. Ouch! We can't just build the output table by hand like describeOneTableDetails does? Admittedly it's kludgy, but it's not an unprecedented kludge. Oh, I had forgotten the existence of that entry point in print.c. Yeah, it might be workable --- want to have a go at it? regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Show INHERIT in \du
On 21/03/2008, Tom Lane [EMAIL PROTECTED] wrote: Brendan Jurd [EMAIL PROTECTED] writes: We can't just build the output table by hand like describeOneTableDetails does? Admittedly it's kludgy, but it's not an unprecedented kludge. Oh, I had forgotten the existence of that entry point in print.c. Yeah, it might be workable --- want to have a go at it? Sure. It will be at least a few days (Easter holidays) before I can submit anything. -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
