ALTER TABLE ... REPLACE WITH
Locked
1234
1234
 
|
|
|
|
|
|
|
|
In reply to this post by Robert Haas
On Thu, Jan 20, 2011 at 09:48:12PM -0500, Robert Haas wrote:
> On Thu, Jan 20, 2011 at 6:19 PM, Noah Misch <[hidden email]> wrote: > > On Thu, Jan 20, 2011 at 09:36:11PM +0000, Simon Riggs wrote: > >> I agree that the DDL behaviour is wrong and should be fixed. Thank you > >> for championing that alternative view. > >> > >> Swapping based upon names only works and is very flexible, much more so > >> than EXCHANGE could be. > >> > >> A separate utility might be worth it, but the feature set of that should > >> be defined in terms of correctly-working DDL behaviour. It's possible > >> that no further requirement exists. I remove my own patch from > >> consideration for this release. > >> > >> I'll review your patch and commit it, problems or objections excepted. I > >> haven't looked at it in any detail. > > > > Thanks. ?I wouldn't be very surprised if that patch is even the wrong way to > > achieve these semantics, but it's great that we're on the same page as to which > > semantics they are. > > I think Noah's patch is a not a good idea, because it will result in > calling RangeVarGetRelid twice even in the overwhelmingly common case > where no relevant invalidation occurs. That'll add several syscache > lookups per table to very common operations. [Refresher: this was a patch to improve behavior for this test case: psql -c "CREATE TABLE t AS VALUES ('old'); CREATE TABLE new_t AS VALUES ('new')" psql -c 'SELECT pg_sleep(2) FROM t' & # block the ALTER or DROP briefly sleep 1 # give prev time to take AccessShareLock # Do it this way, and the next SELECT gets data from the old table. psql -c 'ALTER TABLE t RENAME TO old_t; ALTER TABLE new_t RENAME TO t' & # Do it this way, and get: ERROR: could not open relation with OID 41380 #psql -c 'DROP TABLE t; ALTER TABLE new_t RENAME TO t' & psql -c 'SELECT * FROM t' # I get 'old' or an error, never 'new'. psql -c 'DROP TABLE IF EXISTS t, old_t, new_t' It did so by rechecking the RangeVar->oid resolution after locking the found relation, by which time concurrent DDL could no longer be interfering.] I benchmarked the original patch with this function: Datum nmtest(PG_FUNCTION_ARGS) { int32 n = PG_GETARG_INT32(0); int i; RangeVar *rv = makeRangeVar(NULL, "pg_am", 0); for (i = 0; i < n; ++i) { Relation r = relation_openrv(rv, AccessShareLock); relation_close(r, AccessShareLock); } PG_RETURN_INT32(4); } (Releasing the lock before transaction end makes for an unrealistic benchmark, but so would opening the same relation millions of times in a single transaction. I'm trying to isolate the cost that would be spread over millions of transactions opening relations a handful of times. See attached shar archive for a complete extension wrapping that test function.) Indeed, the original patch slowed it by about 50%. I improved the patch, adding a global SharedInvalidMessageCounter to increment as we process messages. If this counter does not change between the RangeVarGetRelid() call and the post-lock AcceptInvalidationMessages() call, we can skip the second RangeVarGetRelid() call. With the updated patch, I get these timings (in ms) for runs of "SELECT nmtest(10000000)": master: 19697.642, 20087.477, 19748.995 patched: 19723.716, 19879.538, 20257.671 In other words, no significant difference. Since the patch removes the no-longer-needed pre-lock call to AcceptInvalidationMessages(), changing to "relation_close(r, NoLock)" in the test case actually reveals a 15% performance improvement. Granted, nothing to get excited about in light of the artificiality. This semantic improvement would be hard to test with the current pg_regress suite, so I do not include any test case addition in the patch. If sufficiently important, it could be done with isolationtester. Thanks, nm -- Sent via pgsql-hackers mailing list ([hidden email]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers  atomic-openrv-v2.patch (9K) Download Attachment nmtest-relation_openrv.shar (8K) Download Attachment |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
On Sun, Jun 19, 2011 at 11:42 PM, Noah Misch <[hidden email]> wrote:
> On Sun, Jun 19, 2011 at 11:44:35PM +0100, Greg Stark wrote: >> So I was the victim assigned to review this patch. > > Thanks for doing so. This discussion seems to have died off. Let's see if we can drive this forward to some conclusion. I took a look at this patch and found that it had bit-rotted slightly. I am attaching a rebased version. Maybe this is a stupid idea, but what about changing the logic so that, if we get back InvalidOid, we AcceptInvalidationMessages() and retry if the counter has advanced? ISTM that might cover the example you mentioned in your last post, where we fail to detect a relation that has come into existence since our last call to AcceptInvalidationMessages(). It would cost an extra AcceptInvalidationMessages() only in the case where we haven't found the relation, which (a) seems like a good time to worry about whether we're missing something, since users generally try not to reference nonexistent tables and (b) should be rare enough to be ignorable from a performance perspective. In the department of minor nitpicking, why not use a 64-bit counter for SharedInvalidMessageCounter? Then we don't have to think very hard about whether overflow can ever pose a problem. It strikes me that, even with this patch, there is a fair amount of room for wonky behavior. For example, as your comment notes, if search_path = foo, bar, and we've previously referenced "x", getting "bar.x", the creation of "foo.x" will generate invalidation messages, but a subsequent reference - within the same transaction - to "x" will not cause us to read them. It would be nice to AcceptInvalidationMessages() unconditionally at the beginning of RangeVarGetRelid() [and then redo as necessary to get a stable answer], but that might have some performance consequence for transactions that repeatedly read the same tables. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list ([hidden email]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |
|
|
|
|
|
On Wed, Jul 06, 2011 at 08:35:55PM -0400, Robert Haas wrote:
> On Wed, Jul 6, 2011 at 6:32 PM, Noah Misch <[hidden email]> wrote: > > While a mere "LOCK bar.x" is sufficient to get a clean cutover with respect to > > parsing, it fails to invalidate plans. To really cover all bases, you need > > some no-op action that invalidates "bar.x". For actual practical use, I'd > > recommend something like: > > > > BEGIN; > > ALTER TABLE bar.x RENAME TO x0; > > ALTER TABLE bar.x0 RENAME TO x; > > CREATE TABLE foo.x ... > > COMMIT; > > > > Probably worth making it more intuitive to DTRT here. > > Well, what would be really nice is if it just worked. > Care to submit an updated patch? Attached. I made the counter 64 bits wide, handled the nothing-found case per your idea, and improved a few comments cosmetically. I have not attempted to improve the search_path interposition case. We can recommend the workaround above, and doing better looks like an excursion much larger than the one represented by this patch. Thanks, nm -- Sent via pgsql-hackers mailing list ([hidden email]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |
|
|
On Wed, Jul 6, 2011 at 10:44 PM, Noah Misch <[hidden email]> wrote:
> Attached. I made the counter 64 bits wide, handled the nothing-found case per > your idea, and improved a few comments cosmetically. I have not attempted to > improve the search_path interposition case. We can recommend the workaround > above, and doing better looks like an excursion much larger than the one > represented by this patch. I looked at this some more and started to get uncomfortable with the whole idea of having RangeVarLockRelid() be a wrapper around RangeVarGetRelid(). This hazard exists everywhere the latter function gets called, not just in relation_open(). So it doesn't seem right to fix the problem only in those places. So I went through and incorporated the logic proposed for RangeVarLockRelid() into RangeVarGetRelid() itself, and then went through and examined all the callers of RangeVarGetRelid(). There are some, such as has_table_privilege(), where it's really impractical to take any lock, first because we might have no privileges at all on that table and second because that could easily lead to a massive amount of locking for no particular good reason. I believe Tom suggested that the right fix for these functions is to have them index-scan the system catalogs using the caller's MVCC snapshot, which would be right at least for pg_dump. And there are other callers that cannot acquire the lock as part of RangeVarGetRelid() for a variety of other reasons. However, having said that, there do appear to be a number of cases that are can be fixed fairly easily. So here's a (heavily) updated patch that tries to do that, along with adding comments to the places where things still need more fixing. In addition to the problems corrected by your last version, this fixes LOCK TABLE, ALTER SEQUENCE, ALTER TABLE .. RENAME, the whole-table variant of REINDEX, CREATE CONSTRAINT TRIGGER (which is flat-out wrong as it stands, since it acquires *no lock at all* on the table specified in the FROM clause, never mind the question of doing so atomically), CREATE RULE, and (partially) DROP TRIGGER and DROP RULE. Regardless of exactly how we decide to proceed here, it strikes me that there is a heck of a lot more work that could stand to be done in this area... :-( -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list ([hidden email]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |
Loading...
| Powered by Nabble | Edit this page |