From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Noah Misch <noah(at)2ndquadrant(dot)com> |
Cc: | Greg Stark <stark(at)mit(dot)edu>, Simon Riggs <simon(at)2ndquadrant(dot)com>, pgsql-hackers(at)postgresql(dot)org, heikki(dot)linnakangas(at)enterprisedb(dot)com |
Subject: | Re: Make relation_openrv atomic wrt DDL |
Date: | 2011-07-07 00:35:55 |
Message-ID: | CA+TgmoZdQMe-JUO13f_xWpaC=K7eHyME5yi4cy2bMd0sRN1Y-Q@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Jul 6, 2011 at 6:32 PM, Noah Misch <noah(at)2ndquadrant(dot)com> wrote:
>> 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.
>
> Agreed on all points. Good idea. That improves our guarantee from "any
> client-issued command will see tables committed before its submission" to
> "_any command_ will see tables committed before its _parsing_". In
> particular, commands submitted using SPI will no longer be subject to this
> source of déją vu. I, too, doubt that looking up nonexistent relations is a
> performance-critical operation for anyone.
>
>> 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.
>
> Overflow is fine because I only ever compare values for equality, and I use an
> unsigned int to give defined behavior at overflow. However, the added cost of
> a 64-bit counter should be negligible, and future use cases (including
> external code) might appreciate it. No strong preference.
Yeah, that's what I was thinking. I have a feeling we may want to use
this mechanism in other places, including places where it would be
nice to be able to assume that > has sensible semantics.
>> 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.
>
> A user doing that should "LOCK bar.x" in the transaction that creates "foo.x",
> giving a clean cutover. (I thought of documenting that somewhere, but it
> seemed a tad esoteric.) In the absence of such a lock, an extra unconditional
> call to AcceptInvalidationMessages() narrows the window in which his commands
> parse as using the "wrong" table. However, commands that have already parsed
> will still use the old table without interruption, with no particular bound on
> when they may finish. I've failed to come up with a use case where the
> narrower window for parse inconsistencies is valuable but the remaining
> exposure is acceptable. There may very well be one I'm missing, though.
>
> 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?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2011-07-07 00:37:09 | Re: [v9.2] DROP Reworks Part.1 - Consolidate routines to handle DropStmt |
Previous Message | Brar Piening | 2011-07-07 00:26:02 | Re: Review of VS 2010 support patches |