Re: [COMMITTERS] pgsql: Add support for REFRESH MATERIALIZED VIEW CONCURRENTLY. - Mailing list pgsql-hackers

Hi 
On 2013-08-05 08:37:57 -0700, Kevin Grittner wrote:
> Some of the issues raised by Andres and Noah have been addressed.
> These all seemed simple and non-controversial, so I've just applied
> the suggested fixes.

Cool!

> >>>   I'd even suggest using BuildIndexInfo() or such on the indexes,
> >>>   then you could use ->ii_Expressions et al instead of peeking
> >>>   into indkeys by hand.
> >>
> >> Given that the function is in a source file described as containing
> >> "code to create and destroy POSTGRES index relations" and the
> >> comments for that function say that it "stores the information
> >> about the index that's needed by FormIndexDatum, which is used for
> >> both index_build() and later insertion of individual index tuples,"
> >> and we're not going to create or destroy an index or call
> >> FormIndexDatum or insert individual index tuples from this code, it
> >> would seem to be a significant expansion of the charter of that
> >> function.  What benefits do you see to using that level?
> >
> > I'd prevent you from needing to peek into indkeys. Note that it's
> > already used in various places.
> 
> I looked at where it was and wasn't used, and continue to be
> skeptical.  For example, both techniques are used in tablecmds.c;
> the technique you suggest is used where an index is being created,
> dropped, or index tuples are being manipulated, while the
> Form_pg_index structure is being used when the definition of the
> index is being examined without directly manipulating it.  Compare
> what is being done in my code with the existing code for
> ATExecDropNotNull(), for example.

The RelationGetIndexExpressions() you mentioned in the commit sounds
like a good plan to me. Didn't remember that existed.

Don't think the ATExecDropNotNull() comparison is really valid, we need
to know more details there, but anyway, you've found something a good
bit better.

> > What I am thinking of is that you'll get a successfull REFRESH
> > CONCURRENTLY but it will later error out at COMMIT time because there
> > were constraint violations. Afaik we don't have any such behaviour for
> > existing DDL and I don't like introducing it.0
> 
> REFRESH MATERIALIZED VIEW CONCURRENTLY is definitely not DDL.  It
> is DML, and behavior should be consistent with that.  (Note that
> the definition of the matview remains exactly the same after the
> statement executes as it was before; only the data is modified.)
> Without the CONCURRENTLY clause it's in the same sort of gray area
> as TRUNCATE TABLE, where it is essentially DML, but the
> implementation details are similar to that of DDL, so it may
> sometimes be hard to avoid DDL-like behaviors, even though it would
> be best to do so.  We have no such problem here.

But there's no usecase that makes deferred checks and similar useful
afaics. And it seems to me like it certainly will confuse users that a
second RMVC fails (via CheckTableNotInUse()) because there's still a
deferred trigger queue.

Greetings,

Andres Freund

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