Phantom Command IDs, updated patch

Heikki Linnakangas wrote:

Is there any regression tests for the PL languages?

Certainly. See sql and expected directories for each PL. Works using
standard pg_regress.

cheers

andrew

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 10000 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 majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Heikki Linnakangas <heikki@enterprisedb.com> 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

Tom Lane wrote:

Heikki Linnakangas <heikki@enterprisedb.com> 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

Heikki Linnakangas wrote:

Tom Lane wrote:

Heikki Linnakangas <heikki@enterprisedb.com> 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/

Heikki Linnakangas wrote:

Tom Lane wrote:

Heikki Linnakangas <heikki@enterprisedb.com> 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 Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre@commandprompt.com> 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

Tom Lane wrote:

Alvaro Herrera <alvherre@commandprompt.com> 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 <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

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 10000 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 <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

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 10000 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 Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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 +0000; 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

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 +0000; 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 <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> 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

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 10000 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 <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> 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 <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

[ time to move this thread to -hackers ]

Bruce Momjian <bruce@momjian.us> writes:

Tom Lane wrote:

Alvaro Herrera <alvherre@commandprompt.com> 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.

Packed doesn't seem to have quite the right connotation either --- it
sounds like it means there are two separable fields in the CID value.

Maybe "composite cid"?

Another issue that we need to think about before we go too far with this
is the problem that we punted on before 8.2 release: how to deal with
rolling back an upgrade of a row-level lock from shared to exclusive
within a subtransaction. I'm a bit nervous about committing to merging
cmin and cmax before we have an idea how we're going to solve that ---
it might foreclose a solution. Or maybe we could piggyback on phantom/
composite/whatever CIDs to solve it, which would be great, but let's
try to sketch out a solution now.

regards, tom lane

Tom Lane wrote:

[ time to move this thread to -hackers ]

Bruce Momjian <bruce@momjian.us> writes:

Tom Lane wrote:

Alvaro Herrera <alvherre@commandprompt.com> 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.

Packed doesn't seem to have quite the right connotation either --- it
sounds like it means there are two separable fields in the CID value.

Maybe "composite cid"?

At one point I was thinking "combo". but "composite" sounds good.

Another issue that we need to think about before we go too far with this
is the problem that we punted on before 8.2 release: how to deal with
rolling back an upgrade of a row-level lock from shared to exclusive
within a subtransaction. I'm a bit nervous about committing to merging
cmin and cmax before we have an idea how we're going to solve that ---
it might foreclose a solution. Or maybe we could piggyback on phantom/
composite/whatever CIDs to solve it, which would be great, but let's
try to sketch out a solution now.

Good point. Right now we put our new cid on top of the old lock cid,
making rollback impossible to the old lock. What if instead of
overwriting our old cid with a new one, we create a composite cid, and
if we roll back, we look up the composite pair and put the old cid back.
It would only work with two cids, but that seems sufficient.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:

Tom Lane wrote:

Packed doesn't seem to have quite the right connotation either --- it
sounds like it means there are two separable fields in the CID value.

Maybe "composite cid"?

At one point I was thinking "combo". but "composite" sounds good.

I like "combo" --- nice and short.

Another issue that we need to think about before we go too far with this
is the problem that we punted on before 8.2 release: how to deal with
rolling back an upgrade of a row-level lock from shared to exclusive
within a subtransaction. I'm a bit nervous about committing to merging
cmin and cmax before we have an idea how we're going to solve that ---
it might foreclose a solution. Or maybe we could piggyback on phantom/
composite/whatever CIDs to solve it, which would be great, but let's
try to sketch out a solution now.

Good point. Right now we put our new cid on top of the old lock cid,
making rollback impossible to the old lock. What if instead of
overwriting our old cid with a new one, we create a composite cid, and
if we roll back, we look up the composite pair and put the old cid back.
It would only work with two cids, but that seems sufficient.

Yeah, that's more or less what I was thinking. The problem is that the
composite CID isn't going to be enough info to tell you *where* you have
to put things back. And we don't want to try to remember per-row state
in memory. Is there a way to generalize either the composite CID or the
MultiXact mechanism to support this situation without that?

regards, tom lane

Bruce Momjian wrote:

Tom Lane wrote:

Another issue that we need to think about before we go too far with this
is the problem that we punted on before 8.2 release: how to deal with
rolling back an upgrade of a row-level lock from shared to exclusive
within a subtransaction. I'm a bit nervous about committing to merging
cmin and cmax before we have an idea how we're going to solve that ---
it might foreclose a solution. Or maybe we could piggyback on phantom/
composite/whatever CIDs to solve it, which would be great, but let's
try to sketch out a solution now.

Good point. Right now we put our new cid on top of the old lock cid,
making rollback impossible to the old lock. What if instead of
overwriting our old cid with a new one, we create a composite cid, and
if we roll back, we look up the composite pair and put the old cid back.
It would only work with two cids, but that seems sufficient.

This starts to look awfully similar to MultiXactIds. And probably using
such a mechanism would allow you to "rollback" any number of row locks:
take the current membersoof the "multicid", substract the one that
rolled back and use that as new multicid. The main difference is that
you'd need to store both the locker Cid and the mode (shared/exclusive).

The other difference is that multicids can be stored locally to a
backend, no need to have SLRUs etc.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support