Re: Make relation_openrv atomic wrt DDL

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