Re: [PATCHES] [pgsql-patches] Phantom Command IDs, updated patch

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

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(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

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

Heikki Linnakangas <heikki(at)enterprisedb(dot)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(at)enterprisedb(dot)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(at)enterprisedb(dot)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(at)enterprisedb(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

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

Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)us> writes:
> Tom Lane wrote:
>> Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)momjian(dot)us> writes:
> > Tom Lane wrote:
> >> Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

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

Bruce Momjian <bruce(at)momjian(dot)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

Alvaro Herrera wrote:

> 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).

Humm, sorry, obviously this makes no sense at all because I mentally
mixed the Xid locker and the Cids.

> 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/
The PostgreSQL Company - Command Prompt, Inc.

Tom Lane wrote:
> > 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?

Uh, well, hmmm.

The way combo cid is supposed to work is that you are deleting a row
created in your same transaction by a previous command id, so you look
in the combo cid array to see if a match for that pair exists --- if
not, you create a new entry and put the two cids on it.

So, with the combo lock cid, you do the same process, and lookups of who
holds the lock looks at the cid combo, and if the second subtransaction
was aborted, the first one is the lock holder. If you again lock the
row, you create a new combo cid and use the original cid there because
the second cid was aborted.

I don't see how any of this is per-row for locks anymore than it is
per-row for insert/delete.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

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

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Humm, sorry, obviously this makes no sense at all because I mentally
> mixed the Xid locker and the Cids.

After thinking a bit, I have a sketch of a solution.

Assume that we extend the MultiXact infrastructure so that it can track
whether each member of a MultiXact holds shared or exclusive lock.
(There are a couple ways you could do that --- add a parallel bit-array,
or separate the members into two groups. Details not important for now.)
The only way you could have both shared- and exclusive-lock members is
if they are subtransactions of the same backend, but that fact isn't
real relevant to the MultiXact code.

Then extend MultiXactIdWait() so that you can tell it to wait for all
members to die, or just the exclusive members.

Then the representation of the problem situation would be that a locked
tuple would have as XMAX a MultiXact containing the upper XID as shared
locker and the subtransaction as exclusive locker. Onlookers could wait
for one or both to die as appropriate depending on what kind of lock
they needed to get. HEAP_XMAX_EXCL_LOCK would have to be a hint rather
than the truth, ie, once all the exclusive-lock members of the MultiXact
are dead it's really only a shared lock, but I don't see that this poses
any real difficulty.

I don't particularly want to go implement this now; I just want a
proof-of-concept sketch proving that we don't need separate cmin and
cmax to support this.

As for what I think we *should* do near-term, I'm pretty strongly
tempted to suggest that we just throw an error if a subtransaction tries
to upgrade an upper transaction's shared lock to exclusive. When and if
we get a ton of complaints about that, it'd be time to put forth effort
to fix it. I suspect the situation doesn't really arise much in
practice, else we'd have heard complaints from the field about the fact
that the shared lock can become lost.

Comments?

regards, tom lane

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> The way combo cid is supposed to work is that you are deleting a row
> created in your same transaction by a previous command id, so you look
> in the combo cid array to see if a match for that pair exists --- if
> not, you create a new entry and put the two cids on it.

> So, with the combo lock cid, you do the same process, and lookups of who
> holds the lock looks at the cid combo, and if the second subtransaction
> was aborted, the first one is the lock holder. If you again lock the
> row, you create a new combo cid and use the original cid there because
> the second cid was aborted.

No, because no process other than the originator can see the combo-cid
data structure, and for locking situations you really need other
backends to be able to know whether the tuple is locked and how.

But I think my proposal of extending MultiXact would fix it; please look
at that.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > The way combo cid is supposed to work is that you are deleting a row
> > created in your same transaction by a previous command id, so you look
> > in the combo cid array to see if a match for that pair exists --- if
> > not, you create a new entry and put the two cids on it.
>
> > So, with the combo lock cid, you do the same process, and lookups of who
> > holds the lock looks at the cid combo, and if the second subtransaction
> > was aborted, the first one is the lock holder. If you again lock the
> > row, you create a new combo cid and use the original cid there because
> > the second cid was aborted.
>
> No, because no process other than the originator can see the combo-cid
> data structure, and for locking situations you really need other
> backends to be able to know whether the tuple is locked and how.

Oh, OK, I forgot pg_subtrans is visible to all backends.

> But I think my proposal of extending MultiXact would fix it; please look
> at that.

Sounds good.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

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

Heikki Linnakangas <heikki(at)enterprisedb(dot)com> 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

On Thu, 2007-02-08 at 15:32 -0500, Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)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.

Combo is OK, because it's a *combination* of two CommandIds.

That means they are ComboCommandIds or CCIs.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

Simon Riggs wrote:
> On Thu, 2007-02-08 at 15:32 -0500, Tom Lane wrote:
> > Bruce Momjian <bruce(at)momjian(dot)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.
>
> Combo is OK, because it's a *combination* of two CommandIds.
>
> That means they are ComboCommandIds or CCIs.

CCI is CommandCounterIncrement to me, so let's not use that
abbreviation.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

On Fri, 2007-02-09 at 00:08 -0300, Alvaro Herrera wrote:
> Simon Riggs wrote:
> > On Thu, 2007-02-08 at 15:32 -0500, Tom Lane wrote:
> > > Bruce Momjian <bruce(at)momjian(dot)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.
> >
> > Combo is OK, because it's a *combination* of two CommandIds.
> >
> > That means they are ComboCommandIds or CCIs.
>
> CCI is CommandCounterIncrement to me, so let's not use that
> abbreviation.

True; given the similar context that would be a mistake.

Just ComboIds then? I was worried that was cid also.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Simon Riggs wrote:
>> Combo is OK, because it's a *combination* of two CommandIds.
>>
>> That means they are ComboCommandIds or CCIs.

> CCI is CommandCounterIncrement to me, so let's not use that
> abbreviation.

Agreed. I looked for a bit at adding a separate typedef ComboCommandId
to be used where a cid was definitely a combo cid, but it didn't seem to
be very useful to do that...

I'm testing the patch currently. I was a bit surprised to find the
without_oid test failing, but it makes sense because I'm using a
MAXALIGN=8 machine. I suppose Heikki tested on MAXALIGN=4.

regards, tom lane

Heikki Linnakangas <heikki(at)enterprisedb(dot)com> 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

On Thu, 2007-02-08 at 22:15 -0500, Tom Lane wrote:

> I'm testing the patch currently. I was a bit surprised to find the
> without_oid test failing, but it makes sense because I'm using a
> MAXALIGN=8 machine. I suppose Heikki tested on MAXALIGN=4.

That's definitely strange. The patch has been performance tested on a
MAXALIGN=8 system on tables without OIDs on, but I'm not sure we ran the
make check itself on that server. Will do that in future.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

> As for what I think we *should* do near-term, I'm pretty strongly
> tempted to suggest that we just throw an error if a subtransaction
tries
> to upgrade an upper transaction's shared lock to exclusive.

So when a RI check locks a parent, you would not be able to update the
parent
in a later subtrans.
I can imagine, that the error would be a problem in a select for update
loop,
because there you usually want to update the row.

Andreas

Tom Lane wrote:
> I'm testing the patch currently. I was a bit surprised to find the
> without_oid test failing, but it makes sense because I'm using a
> MAXALIGN=8 machine. I suppose Heikki tested on MAXALIGN=4.

That's right.

Thanks for the review!

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

"Zeugswetter Andreas ADI SD" <ZeugswetterA(at)spardat(dot)at> writes:
> So when a RI check locks a parent, you would not be able to update the
> parent in a later subtrans. I can imagine, that the error would be a
> problem in a select for update loop, because there you usually want to
> update the row.

No, it would not, because select for update would acquire exclusive lock
in the first place.

regards, tom lane