Re: Make relation_openrv atomic wrt DDL

On Wed, Jul 06, 2011 at 03:06:40PM -0400, Robert Haas wrote:
> On Sun, Jun 19, 2011 at 11:42 PM, Noah Misch <noah(at)leadboat(dot)com> 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.

Thanks.

> 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.

> 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.

Thanks,
nm