Re: MVCC catalog access

On 2013-06-28 23:14:23 -0400, Robert Haas wrote:
> Here's a further update of this patch.  In this version, I added some
> mechanism to send a new kind of sinval message that is sent when a
> catalog without catcaches is updated; it doesn't apply to all
> catalogs, just to whichever ones we want to have this treatment.  That
> means we don't need to retake snapshots for those catalogs on every
> access, so backend startup requires just one extra MVCC snapshot as
> compared with current master.  Assorted cleanup has been done, along
> with the removal of a few more SnapshotNow references.

This is really cool stuff.

> It's still possible to construct test cases that perform badly by
> pounding the server with 1000 clients running Andres's
> readonly-busy.sql.  Consider the following test case: use a DO block
> to create a schema with 10,000 functions in it and then DROP ..
> CASCADE.  When the server is unloaded, the extra MVCC overhead is
> pretty small.

> Well, now the create is 52% slower and the drop is a whopping 4.7x
> slower.  It's worth digging into the reasons just a bit.  I was able
> to speed up this case quite a bit - it was 30x slower a few hours ago
> - by adding a few new relations to the switch in
> RelationInvalidatesSnapshotsOnly().  But the code still takes one MVCC
> snapshot per object dropped, because deleteOneObject() calls
> CommandCounterIncrement() and that, as it must, invalidates our
> previous snapshot.

I have to say, if the thing that primarily suffers is pretty extreme DDL
in extreme situations I am not really worried. Anybody running anything
close to the territory of such concurrency won't perform that much DDL.

> We could, if we were inclined to spend the effort,
> probably work out that although we need to change curcid, the rest of
> the snapshot is still OK, but I'm not too convinced that it's worth
> adding an even-more-complicated mechanism for this.  We could probably
> also optimize the delete code to increment the command counter fewer
> times, but I'm not convinced that's worth doing either.

I am pretty convinced we shouldn't do either for now.

Something picked up when quickly scanning over the last version of the
patch:

> +/*
> + * Staleness detection for CatalogSnapshot.
> + */
> +static bool CatalogSnapshotStale = true;
>  
>  /*
>   * These are updated by GetSnapshotData.  We initialize them this way
> @@ -177,6 +188,9 @@ GetTransactionSnapshot(void)
>          else
>              CurrentSnapshot = GetSnapshotData(&CurrentSnapshotData);
>  
> +        /* Don't allow catalog snapshot to be older than xact snapshot. */
> +        CatalogSnapshotStale = true;
> +
>          FirstSnapshotSet = true;
>          return CurrentSnapshot;
>      }
> @@ -184,6 +198,9 @@ GetTransactionSnapshot(void)
>      if (IsolationUsesXactSnapshot())
>          return CurrentSnapshot;
>  
> +    /* Don't allow catalog snapshot to be older than xact snapshot. */
> +    CatalogSnapshotStale = true;
> +
>      CurrentSnapshot = GetSnapshotData(&CurrentSnapshotData);
>  
>      return CurrentSnapshot;
> @@ -207,6 +224,54 @@ GetLatestSnapshot(void)
>  }

Do we really need to invalidate snapshots in either situation? Isn't it
implied, that if it's still valid, according to a) no invalidation via local
invalidation messages b) no invalidations from other backends, there
shouldn't be any possible differences when you only look at the catalog?

And if it needs to change, we could copy the newly generated snapshot
to the catalog snapshot if it's currently valid.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services