Thread: Patch that deals with that AtCommit_Portals encounters deleted entries
This patch will ensure that the hash table iteration performed by
AtCommit_Portals is restarted when a portal is deleted. This is
necessary since the deletion of a portal may cause the deletion of
another which on rare occations may cause the iterator to return a
deleted portal an thus a renewed attempt delete.
Regards,
Thomas Hallgren
Index: src/backend/utils/mmgr/portalmem.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/mmgr/portalmem.c,v
retrieving revision 1.76.4.1
diff -u -r1.76.4.1 portalmem.c
--- src/backend/utils/mmgr/portalmem.c 26 Jan 2005 23:20:37 -0000 1.76.4.1
+++ src/backend/utils/mmgr/portalmem.c 27 Feb 2005 18:12:49 -0000
@@ -418,12 +418,6 @@
* materialized form, since we are going to close down the executor and
* release locks. Remove all other portals created in this transaction.
* Portals remaining from prior transactions should be left untouched.
- *
- * XXX This assumes that portals can be deleted in a random order, ie,
- * no portal has a reference to any other (at least not one that will be
- * exercised during deletion). I think this is okay at the moment, but
- * we've had bugs of that ilk in the past. Keep a close eye on cursor
- * references...
*/
void
AtCommit_Portals(void)
@@ -489,6 +483,9 @@
{
/* Zap all non-holdable portals */
PortalDrop(portal, true);
+
+ /* Restart the iteration */
+ hash_seq_init(&status, PortalHashTable);
}
}
}
Thomas Hallgren wrote: > This patch will ensure that the hash table iteration performed by > AtCommit_Portals is restarted when a portal is deleted. This is > necessary since the deletion of a portal may cause the deletion of > another which on rare occations may cause the iterator to return a > deleted portal an thus a renewed attempt delete. I'll apply this to HEAD within 24 hours, barring any objections. -Neil
Neil Conway <neilc@samurai.com> writes:
> Thomas Hallgren wrote:
>> This patch will ensure that the hash table iteration performed by
>> AtCommit_Portals is restarted when a portal is deleted.
> I'll apply this to HEAD within 24 hours, barring any objections.
I don't believe that this actually fixes anything.
In particular, if portal A actually tries to reference portal B during
A's deletion, there is only a 50% chance that this prevents problems.
And since the hash table is traversed in hashcode order, you can't
really imagine that you know which one will be deleted first.
I think a correct fix would involve suppressing that reference in the
first place, rather than making ad-hoc alterations in the traversal
behavior.
regards, tom lane
Tom Lane wrote: >Neil Conway <neilc@samurai.com> writes: > > >>Thomas Hallgren wrote: >> >> >>>This patch will ensure that the hash table iteration performed by >>>AtCommit_Portals is restarted when a portal is deleted. >>> >>> > > > >>I'll apply this to HEAD within 24 hours, barring any objections. >> >> > >I don't believe that this actually fixes anything. > >In particular, if portal A actually tries to reference portal B during >A's deletion, there is only a 50% chance that this prevents problems. >And since the hash table is traversed in hashcode order, you can't >really imagine that you know which one will be deleted first. > > Tom, as I explained to you in an earlier mail exchange, I indeed do cover the cases where B would be deleted before A by intercepting a callback in the portal. So this patch really solve a problem, at least for me. It it would be even better if accompanied by a patch allowing a destroy-callback to be (properly) registered with a Portal. I asked you if that would be something to consider but got no reply. If you do think that's a good idea, please approve this patch for now and I'll deliver another for the callbacks shortly. >I think a correct fix would involve suppressing that reference in the >first place, rather than making ad-hoc alterations in the traversal >behavior. > > I'm not sure what you mean. Earlier you rejected my bug-report on the iterator because you it was the callers responsability to deal with it (hence this patch). Are you now suggesting that we fix that bug instead? Regards, Thomas Hallgren
Thomas Hallgren <thhal@mailblocks.com> writes:
> I'm not sure what you mean. Earlier you rejected my bug-report on the
> iterator because you it was the callers responsability to deal with it
> (hence this patch). Are you now suggesting that we fix that bug instead?
Quite honestly, I dunno. I agree that there's something lacking in this
stack of behaviors, but I don't think any of the solutions suggested so
far actually fix it ...
regards, tom lane
Tom Lane wrote: >Thomas Hallgren <thhal@mailblocks.com> writes: > > >>I'm not sure what you mean. Earlier you rejected my bug-report on the >>iterator because you it was the callers responsability to deal with it >>(hence this patch). Are you now suggesting that we fix that bug instead? >> >> > >Quite honestly, I dunno. I agree that there's something lacking in this >stack of behaviors, but I don't think any of the solutions suggested so >far actually fix it ... > > Ok, in that case, please allow this patch to go through. It will not cause any harm and until a better solution is architected, it does solve my immediate problem. Regards, Thomas Hallgren
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.
---------------------------------------------------------------------------
Thomas Hallgren wrote:
> This patch will ensure that the hash table iteration performed by
> AtCommit_Portals is restarted when a portal is deleted. This is
> necessary since the deletion of a portal may cause the deletion of
> another which on rare occations may cause the iterator to return a
> deleted portal an thus a renewed attempt delete.
>
> Regards,
> Thomas Hallgren
>
> Index: src/backend/utils/mmgr/portalmem.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/backend/utils/mmgr/portalmem.c,v
> retrieving revision 1.76.4.1
> diff -u -r1.76.4.1 portalmem.c
> --- src/backend/utils/mmgr/portalmem.c 26 Jan 2005 23:20:37 -0000 1.76.4.1
> +++ src/backend/utils/mmgr/portalmem.c 27 Feb 2005 18:12:49 -0000
> @@ -418,12 +418,6 @@
> * materialized form, since we are going to close down the executor and
> * release locks. Remove all other portals created in this transaction.
> * Portals remaining from prior transactions should be left untouched.
> - *
> - * XXX This assumes that portals can be deleted in a random order, ie,
> - * no portal has a reference to any other (at least not one that will be
> - * exercised during deletion). I think this is okay at the moment, but
> - * we've had bugs of that ilk in the past. Keep a close eye on cursor
> - * references...
> */
> void
> AtCommit_Portals(void)
> @@ -489,6 +483,9 @@
> {
> /* Zap all non-holdable portals */
> PortalDrop(portal, true);
> +
> + /* Restart the iteration */
> + hash_seq_init(&status, PortalHashTable);
> }
> }
> }
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Thomas Hallgren wrote: > This patch will ensure that the hash table iteration performed by > AtCommit_Portals is restarted when a portal is deleted. This is > necessary since the deletion of a portal may cause the deletion of > another which on rare occations may cause the iterator to return a > deleted portal an thus a renewed attempt delete. I have applied the following patch. I assume it is too risky for backpatch to 8.0.X. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073 Index: src/backend/utils/mmgr/portalmem.c =================================================================== RCS file: /cvsroot/pgsql/src/backend/utils/mmgr/portalmem.c,v retrieving revision 1.78 diff -c -c -r1.78 portalmem.c *** src/backend/utils/mmgr/portalmem.c 11 Apr 2005 19:51:15 -0000 1.78 --- src/backend/utils/mmgr/portalmem.c 11 May 2005 18:03:38 -0000 *************** *** 475,486 **** * * Remove all non-holdable portals created in this transaction. * Portals remaining from prior transactions should be left untouched. - * - * XXX This assumes that portals can be deleted in a random order, ie, - * no portal has a reference to any other (at least not one that will be - * exercised during deletion). I think this is okay at the moment, but - * we've had bugs of that ilk in the past. Keep a close eye on cursor - * references... */ void AtCommit_Portals(void) --- 475,480 ---- *************** *** 516,521 **** --- 510,518 ---- /* Zap all non-holdable portals */ PortalDrop(portal, true); + + /* Restart the iteration */ + hash_seq_init(&status, PortalHashTable); } }
Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> This patch will ensure that the hash table iteration performed by
>> AtCommit_Portals is restarted when a portal is deleted.
> I have applied the following patch. I assume it is too risky for
> backpatch to 8.0.X.
I don't think it's appropriate in HEAD either --- it doesn't solve the
problem, and it is an expensive way of not doing so :-(
regards, tom lane
Tom Lane wrote: >Bruce Momjian <pgman@candle.pha.pa.us> writes: > > >>>This patch will ensure that the hash table iteration performed by >>>AtCommit_Portals is restarted when a portal is deleted. >>> >>> > > > >>I have applied the following patch. I assume it is too risky for >>backpatch to 8.0.X. >> >> > >I don't think it's appropriate in HEAD either --- it doesn't solve the >problem, and it is an expensive way of not doing so :-( > > It certanly solves my problem! And as for expensive? Why would it be? It just restarts the iteration everytime something is removed. The only thing that could make this patch consume CPU cycles is if there's a vast amount of Portals in the iteration that are not candidates for removal. I don't see that happen. Do you? Regards, Thomas Hallgren