Re: [PATCHES] [pgsql-patches] Phantom Command IDs, updated patch
Heikki Linnakangas <[EMAIL PROTECTED]> writes: > Here's an updated version of the phantom command ids patch. Applied with some revisions (notably, renaming 'em to "combo" command IDs). regards, tom lane ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] [pgsql-patches] Phantom Command IDs, updated patch
Heikki Linnakangas <[EMAIL PROTECTED]> writes: > I found one more subtle safety issue. The array and hash table for > phantom command ids is dynamically grown when HeapTupleHeaderSetCmax is > called. Unfortunately, since HeapTupleHeaderSetCmax is used inside a > critical sections, running out of memory while trying to grow them would > cause a PANIC. That's why I moved the SetXmax/SetCmax calls outside > critical sections in heapam.c. I believe that's safe; if a backend > aborts after setting the xmax/cmax, no-one is going to care about the > xid of an aborted transaction in there. I don't like that one bit; I think a saner solution is to refactor the API of combocid.c so that we can obtain the required CID before modifying the page. It'll mean it's not a drop-in replacement for HeapTupleSetCmax, but all callers of that are going to need a close look anyway. regards, tom lane ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [pgsql-patches] Phantom Command IDs, updated patch
Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> writes: > > Heikki, I found something odd in your patch. You had an extra > > parentheses at the end of the line in the orginal and new version of the > > patch (attached). I removed it before applying, but I just wanted to > > confirm this was OK. > > Please do not apply that patch --- I want to review it first. OK. -- Bruce Momjian <[EMAIL PROTECTED]> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] [pgsql-patches] Phantom Command IDs, updated patch
Alvaro Herrera wrote: > > > Per Tom's suggestion, I replaced the function cache code in fmgr.c and > > > similar code in plperl.c, pltcl.c, plpgsql/pl_comp.c and plpython.c to > > > use xmin+tid instead of xmin+cmin for the up-to-dateness check. I don't > > > have any tcl, perl or python test cases handy to test them, but the > > > change is small and essentially same for all of the above. Is there any > > > regression tests for the PL languages? > > > > > > I made cmin and cmax system attributes aliases for the same physical > > > commandid field. I support the idea of a complete overhaul of those > > > system attributes, but let's do that in a separate patch. > > > > > > To measure the overhead, I ran a plpgsql test case that updates a single > > > row 1 times in a loop, generating a new phantom command id in each > > > iteration. The test took ~5% longer with the patch, so I think that's > > > acceptable. I couldn't measure a difference with pgbench (as expected). > > > > > > I think the patch is ready. Please remove the PHANTOMCID_DEBUG define > > > and ifdef blocks before applying. > > > > Heikki, I found something odd in your patch. You had an extra > > parentheses at the end of the line in the orginal and new version of the > > patch (attached). I removed it before applying, but I just wanted to > > confirm this was OK. > > Huh, you already applied it? When I said "applying", I meant before applying the patch to my CVS tree, not before commiting, because I didn't commit it. -- Bruce Momjian <[EMAIL PROTECTED]> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] [pgsql-patches] Phantom Command IDs, updated patch
Bruce Momjian <[EMAIL PROTECTED]> writes: > Heikki, I found something odd in your patch. You had an extra > parentheses at the end of the line in the orginal and new version of the > patch (attached). I removed it before applying, but I just wanted to > confirm this was OK. Please do not apply that patch --- I want to review it first. regards, tom lane ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [pgsql-patches] Phantom Command IDs, updated patch
Heikki Linnakangas wrote: > Bruce Momjian wrote: > > Heikki, I found something odd in your patch. You had an extra > > parentheses at the end of the line in the orginal and new version of the > > patch (attached). I removed it before applying, but I just wanted to > > confirm this was OK. > > Looking at the CVS history, it looks like Tom changed that piece of code > recently in this commit: > > > revision 1.110 > > date: 2007-01-30 22:05:12 +; author: tgl; state: Exp; lines: +88 -21; > > Repair oversights in the mechanism used to store compiled plpgsql functions. > > The original coding failed (tried to access deallocated memory) if there > > were > > two active call sites (fn_extra pointers) for the same function and the > > function definition was updated. Also, if an update of a recursive function > > was detected upon nested entry to the function, the existing compiled > > version > > was summarily deallocated, resulting in crash upon return to the outer > > instance. Problem observed while studying a bug report from Sergiy > > Vyshnevetskiy. > > > > Bug does not exist before 8.1 since older versions just leaked the memory of > > obsoleted compiled functions, rather than trying to reclaim it. > > Note that the condition in the if-clause is now the other way round, and > the delete_function call is now in the else-branch. Did you get that > right in your commit? No, I did not see that, but I see it now. I haven't committed anything yet. I will research that and get it right. -- Bruce Momjian <[EMAIL PROTECTED]> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [pgsql-patches] Phantom Command IDs, updated patch
Bruce Momjian wrote: Heikki, I found something odd in your patch. You had an extra parentheses at the end of the line in the orginal and new version of the patch (attached). I removed it before applying, but I just wanted to confirm this was OK. Looking at the CVS history, it looks like Tom changed that piece of code recently in this commit: revision 1.110 date: 2007-01-30 22:05:12 +; author: tgl; state: Exp; lines: +88 -21; Repair oversights in the mechanism used to store compiled plpgsql functions. The original coding failed (tried to access deallocated memory) if there were two active call sites (fn_extra pointers) for the same function and the function definition was updated. Also, if an update of a recursive function was detected upon nested entry to the function, the existing compiled version was summarily deallocated, resulting in crash upon return to the outer instance. Problem observed while studying a bug report from Sergiy Vyshnevetskiy. Bug does not exist before 8.1 since older versions just leaked the memory of obsoleted compiled functions, rather than trying to reclaim it. Note that the condition in the if-clause is now the other way round, and the delete_function call is now in the else-branch. Did you get that right in your commit? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] [pgsql-patches] Phantom Command IDs, updated patch
Bruce Momjian wrote: > Heikki Linnakangas wrote: > > Here's an updated version of the phantom command ids patch. > > > > I found one more subtle safety issue. The array and hash table for > > phantom command ids is dynamically grown when HeapTupleHeaderSetCmax is > > called. Unfortunately, since HeapTupleHeaderSetCmax is used inside a > > critical sections, running out of memory while trying to grow them would > > cause a PANIC. That's why I moved the SetXmax/SetCmax calls outside > > critical sections in heapam.c. I believe that's safe; if a backend > > aborts after setting the xmax/cmax, no-one is going to care about the > > xid of an aborted transaction in there. > > > > Per Tom's suggestion, I replaced the function cache code in fmgr.c and > > similar code in plperl.c, pltcl.c, plpgsql/pl_comp.c and plpython.c to > > use xmin+tid instead of xmin+cmin for the up-to-dateness check. I don't > > have any tcl, perl or python test cases handy to test them, but the > > change is small and essentially same for all of the above. Is there any > > regression tests for the PL languages? > > > > I made cmin and cmax system attributes aliases for the same physical > > commandid field. I support the idea of a complete overhaul of those > > system attributes, but let's do that in a separate patch. > > > > To measure the overhead, I ran a plpgsql test case that updates a single > > row 1 times in a loop, generating a new phantom command id in each > > iteration. The test took ~5% longer with the patch, so I think that's > > acceptable. I couldn't measure a difference with pgbench (as expected). > > > > I think the patch is ready. Please remove the PHANTOMCID_DEBUG define > > and ifdef blocks before applying. > > Heikki, I found something odd in your patch. You had an extra > parentheses at the end of the line in the orginal and new version of the > patch (attached). I removed it before applying, but I just wanted to > confirm this was OK. Huh, you already applied it? -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] [pgsql-patches] Phantom Command IDs, updated patch
Heikki Linnakangas wrote:
> Here's an updated version of the phantom command ids patch.
>
> I found one more subtle safety issue. The array and hash table for
> phantom command ids is dynamically grown when HeapTupleHeaderSetCmax is
> called. Unfortunately, since HeapTupleHeaderSetCmax is used inside a
> critical sections, running out of memory while trying to grow them would
> cause a PANIC. That's why I moved the SetXmax/SetCmax calls outside
> critical sections in heapam.c. I believe that's safe; if a backend
> aborts after setting the xmax/cmax, no-one is going to care about the
> xid of an aborted transaction in there.
>
> Per Tom's suggestion, I replaced the function cache code in fmgr.c and
> similar code in plperl.c, pltcl.c, plpgsql/pl_comp.c and plpython.c to
> use xmin+tid instead of xmin+cmin for the up-to-dateness check. I don't
> have any tcl, perl or python test cases handy to test them, but the
> change is small and essentially same for all of the above. Is there any
> regression tests for the PL languages?
>
> I made cmin and cmax system attributes aliases for the same physical
> commandid field. I support the idea of a complete overhaul of those
> system attributes, but let's do that in a separate patch.
>
> To measure the overhead, I ran a plpgsql test case that updates a single
> row 1 times in a loop, generating a new phantom command id in each
> iteration. The test took ~5% longer with the patch, so I think that's
> acceptable. I couldn't measure a difference with pgbench (as expected).
>
> I think the patch is ready. Please remove the PHANTOMCID_DEBUG define
> and ifdef blocks before applying.
Heikki, I found something odd in your patch. You had an extra
parentheses at the end of the line in the orginal and new version of the
patch (attached). I removed it before applying, but I just wanted to
confirm this was OK.
--
Bruce Momjian <[EMAIL PROTECTED]> http://momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
***
*** 162,168
{
/* We have a compiled function, but is it still valid? */
if (!(function->fn_xmin ==
HeapTupleHeaderGetXmin(procTup->t_data) &&
! function->fn_cmin ==
HeapTupleHeaderGetCmin(procTup->t_data)))
{
/* Nope, drop the function and associated storage */
delete_function(function);
--- 162,168
{
/* We have a compiled function, but is it still valid? */
if (!(function->fn_xmin ==
HeapTupleHeaderGetXmin(procTup->t_data) &&
! ItemPointerEquals(&function->fn_tid,
&procTup->t_self)))
{
/* Nope, drop the function and associated storage */
delete_function(function);
---(end of broadcast)---
TIP 7: You can help support the PostgreSQL project by donating at
http://www.postgresql.org/about/donate
Re: [PATCHES] [pgsql-patches] Phantom Command IDs, updated patch
Tom Lane wrote: > Alvaro Herrera <[EMAIL PROTECTED]> writes: > > Heikki Linnakangas wrote: > >> Tom Lane wrote: > >>> BTW, I don't care much for the terminology "phantom cid" ... there's > >>> nothing particularly "phantom" about them, seeing they get onto disk. > >>> Can anyone think of a better name? The best I can do offhand is > >>> "merged cid" or "cid pair", which aren't inspiring. > >> > >> MultiCid, like the MultiXacts? Maybe not, they're quite different beasts... > > > Dual cid? Double cid? > > "Double cid" doesn't sound too bad. Another thought that just came to > mind is "cid interval" or some variant of that. I don't like "double ctid" because it is really just one ctid, but represents two. I am thinking "packed ctid" is the right wording. It doesn't have the same impact as "phantom", but it is probably better. -- Bruce Momjian <[EMAIL PROTECTED]> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] [pgsql-patches] Phantom Command IDs, updated patch
Alvaro Herrera <[EMAIL PROTECTED]> writes: > Heikki Linnakangas wrote: >> Tom Lane wrote: >>> BTW, I don't care much for the terminology "phantom cid" ... there's >>> nothing particularly "phantom" about them, seeing they get onto disk. >>> Can anyone think of a better name? The best I can do offhand is >>> "merged cid" or "cid pair", which aren't inspiring. >> >> MultiCid, like the MultiXacts? Maybe not, they're quite different beasts... > Dual cid? Double cid? "Double cid" doesn't sound too bad. Another thought that just came to mind is "cid interval" or some variant of that. regards, tom lane ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] [pgsql-patches] Phantom Command IDs, updated patch
Heikki Linnakangas wrote: > Tom Lane wrote: > >Heikki Linnakangas <[EMAIL PROTECTED]> writes: > >>I think the patch is ready. Please remove the PHANTOMCID_DEBUG define > >>and ifdef blocks before applying. > > > >BTW, I don't care much for the terminology "phantom cid" ... there's > >nothing particularly "phantom" about them, seeing they get onto disk. > >Can anyone think of a better name? The best I can do offhand is > >"merged cid" or "cid pair", which aren't inspiring. > > MultiCid, like the MultiXacts? Maybe not, they're quite different beasts... Dual cid? Double cid? -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] [pgsql-patches] Phantom Command IDs, updated patch
Heikki Linnakangas wrote: > Tom Lane wrote: >> Heikki Linnakangas <[EMAIL PROTECTED]> writes: >>> I think the patch is ready. Please remove the PHANTOMCID_DEBUG define >>> and ifdef blocks before applying. >> >> BTW, I don't care much for the terminology "phantom cid" ... there's >> nothing particularly "phantom" about them, seeing they get onto disk. >> Can anyone think of a better name? The best I can do offhand is >> "merged cid" or "cid pair", which aren't inspiring. > > MultiCid, like the MultiXacts? Maybe not, they're quite different beasts... > > Alias cid? Mapped cid? Compressed cid? Hero cid? :) > > I'm happy with phantom cid myself. It sounds cool, and they are a bit > phantom-like because the true meaning of a phantom cid is lost when the > transaction ends. > Phantom was also a super hero ;) Sincerely, Joshua D. Drake -- === The PostgreSQL Company: Command Prompt, Inc. === Sales/Support: +1.503.667.4564 || 24x7/Emergency: +1.800.492.2240 Providing the most comprehensive PostgreSQL solutions since 1997 http://www.commandprompt.com/ Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate PostgreSQL Replication: http://www.commandprompt.com/products/ ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] [pgsql-patches] Phantom Command IDs, updated patch
Tom Lane wrote: Heikki Linnakangas <[EMAIL PROTECTED]> writes: I think the patch is ready. Please remove the PHANTOMCID_DEBUG define and ifdef blocks before applying. BTW, I don't care much for the terminology "phantom cid" ... there's nothing particularly "phantom" about them, seeing they get onto disk. Can anyone think of a better name? The best I can do offhand is "merged cid" or "cid pair", which aren't inspiring. MultiCid, like the MultiXacts? Maybe not, they're quite different beasts... Alias cid? Mapped cid? Compressed cid? Hero cid? :) I'm happy with phantom cid myself. It sounds cool, and they are a bit phantom-like because the true meaning of a phantom cid is lost when the transaction ends. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] [pgsql-patches] Phantom Command IDs, updated patch
Heikki Linnakangas <[EMAIL PROTECTED]> writes: > I think the patch is ready. Please remove the PHANTOMCID_DEBUG define > and ifdef blocks before applying. BTW, I don't care much for the terminology "phantom cid" ... there's nothing particularly "phantom" about them, seeing they get onto disk. Can anyone think of a better name? The best I can do offhand is "merged cid" or "cid pair", which aren't inspiring. Now would be a good time to change it while it'd still be an easy search-and-replace over a patch file ... regards, tom lane ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] [pgsql-patches] Phantom Command IDs, updated patch
Your patch has been added to the PostgreSQL unapplied patches list at: http://momjian.postgresql.org/cgi-bin/pgpatches It will be applied as soon as one of the PostgreSQL committers reviews and approves it. --- Heikki Linnakangas wrote: > Here's an updated version of the phantom command ids patch. > > I found one more subtle safety issue. The array and hash table for > phantom command ids is dynamically grown when HeapTupleHeaderSetCmax is > called. Unfortunately, since HeapTupleHeaderSetCmax is used inside a > critical sections, running out of memory while trying to grow them would > cause a PANIC. That's why I moved the SetXmax/SetCmax calls outside > critical sections in heapam.c. I believe that's safe; if a backend > aborts after setting the xmax/cmax, no-one is going to care about the > xid of an aborted transaction in there. > > Per Tom's suggestion, I replaced the function cache code in fmgr.c and > similar code in plperl.c, pltcl.c, plpgsql/pl_comp.c and plpython.c to > use xmin+tid instead of xmin+cmin for the up-to-dateness check. I don't > have any tcl, perl or python test cases handy to test them, but the > change is small and essentially same for all of the above. Is there any > regression tests for the PL languages? > > I made cmin and cmax system attributes aliases for the same physical > commandid field. I support the idea of a complete overhaul of those > system attributes, but let's do that in a separate patch. > > To measure the overhead, I ran a plpgsql test case that updates a single > row 1 times in a loop, generating a new phantom command id in each > iteration. The test took ~5% longer with the patch, so I think that's > acceptable. I couldn't measure a difference with pgbench (as expected). > > I think the patch is ready. Please remove the PHANTOMCID_DEBUG define > and ifdef blocks before applying. > > -- >Heikki Linnakangas >EnterpriseDB http://www.enterprisedb.com > > ---(end of broadcast)--- > TIP 1: if posting/reading through Usenet, please send an appropriate >subscribe-nomail command to [EMAIL PROTECTED] so that your >message can get through to the mailing list cleanly -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
