Re: [PATCHES] TODO-item: Add sleep() function, remove from regress.c
Joachim Wieland [EMAIL PROTECTED] writes: On Mon, Jan 09, 2006 at 08:50:36PM -0500, Tom Lane wrote: The proposed regression test seems unacceptably fragile, as well as rather pointless. Why is it fragile? As your own comment pointed out, the test would fail on a heavily loaded system, due to sleeping too long. I do not see the need for such a test anyway --- the stats regression test will exercise the code sufficiently. BTW, a serious problem with just passing it off to pg_usleep like that is that the sleep can't be aborted by a cancel request No, cancelling the sleep works (at least for Unix). Isn't cancelling implemented via a signal that interrupts select() ? On some systems the signal will interrupt select. On some, not. Note the coding in bgwriter.c's main loop for instance. regards, tom lane ---(end of broadcast)--- TIP 6: explain analyze is your friend
[PATCHES] TupleDesc refcounting
Attached is a WIP patch that adds reference counting for TupleDescs. Two issues that I ran into while implementing it: (1) How should the lifetime of a TupleDesc be managed? The existing ResourceOwner stuff seems to assume that it is managing per-query resources. A TupleDesc will often live beyond the lifetime of a single transaction, and might even be created before transactions can be started (e.g. formrdesc() in relcache.c, when we're creating relcache entries for nailed-in-cache relations early in InitPostgres()). In current sources, the lifetime of a TupleDesc is the lifetime of the memory context in which it is allocated (and/or whenever FreeTupleDesc() is invoked). We could imitate that behavior by optionally linking a ResourceOwner with each MemoryContext, and releasing the ResourceOwner when the MemoryContext is reset or deleted. However, I'm not sure that that's the right approach... (2) The existing ResourceOwner users issue a warning if the resource they are managing is not explicitly released before a transaction successfully commits (so they elog(WARNING)). I don't see the need to be that strict for TupleDescs -- as we do with palloc() without a matching pfree(), I think it should be okay to just silently clean up leaked TupleDescs when releasing a ResourceOwner. Thoughts? -Neil *** src/backend/access/common/tupdesc.c 83ca807d4fdd572c409bc9214922b6ba9da7ce18 --- src/backend/access/common/tupdesc.c 63efd0b6adc911b57cef00cf0c4611760a91fa2c *** *** 23,28 --- 23,29 #include catalog/pg_type.h #include parser/parse_type.h #include utils/builtins.h + #include utils/resowner.h #include utils/syscache.h *** *** 30,37 * CreateTemplateTupleDesc * This function allocates an empty tuple descriptor structure. * ! * Tuple type ID information is initially set for an anonymous record type; ! * caller can overwrite this if needed. */ TupleDesc CreateTemplateTupleDesc(int natts, bool hasoid) --- 31,42 * CreateTemplateTupleDesc * This function allocates an empty tuple descriptor structure. * ! * Tuple type ID information is initially set for an anonymous record ! * type; caller can overwrite this if needed. ! * ! * The reference count on the TupleDesc is initially 1: the caller ! * should decrement the reference count via DecrTupleDescRefCount() ! * when finished. */ TupleDesc CreateTemplateTupleDesc(int natts, bool hasoid) *** *** 85,90 --- 90,99 desc-tdtypmod = -1; desc-tdhasoid = hasoid; + /* Set to zero, then inform the resowner and increment refcount */ + desc-refcount = 0; + IncrTupleDescRefCount(desc); + return desc; } *** *** 93,103 * This function allocates a new TupleDesc pointing to a given * Form_pg_attribute array. * ! * Note: if the TupleDesc is ever freed, the Form_pg_attribute array * will not be freed thereby. * * Tuple type ID information is initially set for an anonymous record type; * caller can overwrite this if needed. */ TupleDesc CreateTupleDesc(int natts, bool hasoid, Form_pg_attribute *attrs) --- 102,116 * This function allocates a new TupleDesc pointing to a given * Form_pg_attribute array. * ! * Note: when the TupleDesc is destroyed, the Form_pg_attribute array * will not be freed thereby. * * Tuple type ID information is initially set for an anonymous record type; * caller can overwrite this if needed. + * + * The reference count on the TupleDesc is initially 1: the caller + * should decrement the reference count via DecrTupleDescRefCount() + * when finished. */ TupleDesc CreateTupleDesc(int natts, bool hasoid, Form_pg_attribute *attrs) *** *** 117,122 --- 130,139 desc-tdtypmod = -1; desc-tdhasoid = hasoid; + /* Set to zero, then inform the resowner and increment refcount */ + desc-refcount = 0; + IncrTupleDescRefCount(desc); + return desc; } *** *** 125,130 --- 142,151 * This function creates a new TupleDesc by copying from an existing * TupleDesc. * + * The reference count on the TupleDesc is initially 1: the caller + * should decrement the reference count via DecrTupleDescRefCount() + * when finished. + * * !!! Constraints and defaults are not copied !!! */ TupleDesc *** *** 144,150 desc-tdtypeid = tupdesc-tdtypeid; desc-tdtypmod = tupdesc-tdtypmod; ! return desc; } --- 165,171 desc-tdtypeid = tupdesc-tdtypeid; desc-tdtypmod = tupdesc-tdtypmod; ! return desc; } *** *** 152,157 --- 173,182 * CreateTupleDescCopyConstr * This function creates a new TupleDesc by copying from an existing * TupleDesc (including its constraints and defaults). + * + * The reference count on the TupleDesc is initially 1: the caller +
Re: [PATCHES] TupleDesc refcounting
Neil Conway [EMAIL PROTECTED] writes: (1) How should the lifetime of a TupleDesc be managed? The existing ResourceOwner stuff seems to assume that it is managing per-query resources. Yeah. I was envisioning two different approaches: for TupleDesc references from long-lived data structures (ie, typcache or relcache) just increment or decrement the count when the referencing data structure changes. ResourceOwner would be used for dynamic within-query references --- in practice, local variables. If the reference would be forgotten during an elog longjmp then you need a ResourceOwner to backstop it, otherwise not. You could conceivably set up a cache ResourceOwner with indefinite lifespan to make the cache case more like the local-variable case, but I think this would merely be a performance drag with no real value. There's no point in a ResourceOwner if it will never be called on to release resources, and a cache-lifespan ResourceOwner wouldn't be. In current sources, the lifetime of a TupleDesc is the lifetime of the memory context in which it is allocated (and/or whenever FreeTupleDesc() is invoked). We could imitate that behavior by optionally linking a ResourceOwner with each MemoryContext, and releasing the ResourceOwner when the MemoryContext is reset or deleted. However, I'm not sure that that's the right approach... No. We really only need this mechanism for the case where a tupledesc allocated in a long-lived context (again, think relcache or typcache) is going to be dynamically referenced by shorter-lived code. Tying it to MemoryContexts won't help because CacheMemoryContext never gets reset. (2) The existing ResourceOwner users issue a warning if the resource they are managing is not explicitly released before a transaction successfully commits (so they elog(WARNING)). I don't see the need to be that strict for TupleDescs -- as we do with palloc() without a matching pfree(), I think it should be okay to just silently clean up leaked TupleDescs when releasing a ResourceOwner. The reason why those warnings are issued is to catch code that is failing to manage references properly. I think that motivation applies perfectly well to tuple descriptors too. There is no good reason for code to forget to release the descriptor reference in non-error paths. No time to look at the patch itself right now ... regards, tom lane ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] Summary table trigger example race condition
On Sun, Jan 08, 2006 at 04:13:01PM +1300, Mark Kirkwood wrote: After re-examining the original code, it looks like it was not actually vulnerable to a race condition! (it does the UPDATE, then if not found will do an INSERT, and handle unique violation with a repeat of the same UPDATE - i.e three DML statements, which are enough to handle the race in this case). What happens if someone deletes the row between the failed insert and the second update? :) AFAICT, example 36-1 is the only way to handle this without creating a race condition. However Jim's change handles the race needing only two DML statements in a loop, which seems much more elegant! In addition it provides a nice example of the 'merge' style code shown in e.g 36-1. What's SOP here... should I ping someone to let them know this patch should be committed now that those who care are happy with it? -- Jim C. Nasby, Sr. Engineering Consultant [EMAIL PROTECTED] Pervasive Software http://pervasive.comwork: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461 ---(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] Summary table trigger example race condition
Jim C. Nasby wrote: On Sun, Jan 08, 2006 at 04:13:01PM +1300, Mark Kirkwood wrote: After re-examining the original code, it looks like it was not actually vulnerable to a race condition! (it does the UPDATE, then if not found will do an INSERT, and handle unique violation with a repeat of the same UPDATE - i.e three DML statements, which are enough to handle the race in this case). What happens if someone deletes the row between the failed insert and the second update? :) In this example the rows in the summary table never get deleted by DELETE operations on that main one - the trigger just decrements the various amounts - i.e DELETE becomes UPDATE, so no problem there. AFAICT, example 36-1 is the only way to handle this without creating a race condition. For the general case indeed you are correct - a good reason for this change :-). In addition, that fact that it is actually quite difficult to be sure that any race condition is actually being handled makes (another) good reason for using the most robust method in the 'official' examples. Best wishes Mark ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] TODO-item: Add sleep() function, remove from regress.c
On Tue, Jan 10, 2006 at 11:31:13AM +0100, Joachim Wieland wrote:
No, cancelling the sleep works (at least for Unix). Isn't cancelling
implemented via a signal that interrupts select() ?
Anyway, I've changed it, removing the ~2000s limit is a good point.
+ while (secs 1.0)
+ {
+ pg_usleep(100);
+ CHECK_FOR_INTERRUPTS();
+ secs -= 1.0;
+ }
Won't this result in a call to pg_sleep with a long sleep time ending up
sleeping noticeably longer than requested?
--
Jim C. Nasby, Sr. Engineering Consultant [EMAIL PROTECTED]
Pervasive Software http://pervasive.comwork: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
---(end of broadcast)---
TIP 6: explain analyze is your friend
Re: [PATCHES] Summary table trigger example race condition
Mark Kirkwood wrote: Jim C. Nasby wrote: On Sun, Jan 08, 2006 at 04:13:01PM +1300, Mark Kirkwood wrote: What happens if someone deletes the row between the failed insert and the second update? :) In this example the rows in the summary table never get deleted by DELETE operations on that main one - the trigger just decrements the various amounts - i.e DELETE becomes UPDATE, so no problem there. Sorry Jim, I just realized you probably meant someone directly deleting rows in the summary table itself. Well yes, that would certainly fox it! I guess I was implicitly considering a use case where the only direct DML on the summary table would be some sort of ETL process, which would probably lock out other changes (and re-create the summary table data afterwards in all likelihood). Cheers Mark ---(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] TODO-item: Add sleep() function, remove from regress.c
Andrew Dunstan [EMAIL PROTECTED] writes: Jim C. Nasby wrote: Won't this result in a call to pg_sleep with a long sleep time ending up sleeping noticeably longer than requested? Looks like it to me. Something on the order of 1% longer, hm? (1 extra clock tick per second, probably.) Can't get excited about it --- *all* implementations of sleep say that the time is minimum not exact. regards, tom lane ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] TODO-item: Add sleep() function, remove from regress.c
Tom Lane said: Andrew Dunstan [EMAIL PROTECTED] writes: Jim C. Nasby wrote: Won't this result in a call to pg_sleep with a long sleep time ending up sleeping noticeably longer than requested? Looks like it to me. Something on the order of 1% longer, hm? (1 extra clock tick per second, probably.) Can't get excited about it --- *all* implementations of sleep say that the time is minimum not exact. Well yes, although it's cumulative. I guess I'm not excited for a different reason - I'm having trouble imagining much of a use case. cheers andrew ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
