Kevin Grittner <kgri...@ymail.com> wrote:
> I think a large part of what is at issue here stems from a bad
> name for the new bool field I added to the RelationData structure
> -- instead of rd_isscannable it should probably be called
> something like rd_ispopulated. The current name led to some
> fuzzy thinking on my part when it was referenced, and both the
> name and a couple ill-considered uses of it probably contributed
> to concerns about how it was generated and used.
>
> I will post a draft patch today to see whether concerns abate.
> Basically, the name change should help make clear that this is
> not intended to be the only way to determine whether a matview is
> scannable. Second, there is at least on (and probably more)
> direct tests of this field which should use a function for a
> scannability test. For 9.3, that will just wrap a test of this
> bool, but it makes clear what the longer-term intent is, and help
> ensure that things don't get missed when patches are written in
> later releases. Third, some comments need to be corrected and
> added.
>
> Hopefully it can help get us all onto the same page. If not, it
> should at least better focus the discussion.
Attached is a firt cut at drawing a bright line between the notion
of whether a matview has been *populated* and whether it is
*scannable*. In 9.3 one is true if and only if the other is, but
I plan on doing work such that this will no longer be true in the
next release, and not making a clear distinction now has been
confusing everyone, including me.
This patch is light on functional changes, and heavier on name and
comment changes. It does fix the lack of locking for the
user-visible pg_relation_is_scannable() function, and it does
correct the performance regression in pg_dump, but those should be
the only user-visible changes. Internally I also tried to correct
a modularity violation complained of by Tom.
Vacuum still needs to be taught not to truncate away the first page
of a matview, but that seemed like it was better left for a
separate patch, and if we decide not to use the current technique
for identifying a non-populated matview, it owuld be one more thing
to undo.
It passed `make check-world` and `make installcheck-world`, and a
dump and load of the regression database works. I got sidetracked
with some support issues, so I didn't have time to dig around for
other weak comments, but I think I'm close on all other changes.
Comments?
--
Kevin Grittner
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
*** a/src/backend/commands/cluster.c
--- b/src/backend/commands/cluster.c
***************
*** 381,393 **** cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose,
check_index_is_clusterable(OldHeap, indexOid, recheck, AccessExclusiveLock);
/*
! * Quietly ignore the request if the a materialized view is not scannable.
! * No harm is done because there is nothing no data to deal with, and we
! * don't want to throw an error if this is part of a multi-relation
! * request -- for example, CLUSTER was run on the entire database.
*/
if (OldHeap->rd_rel->relkind == RELKIND_MATVIEW &&
! !OldHeap->rd_isscannable)
{
relation_close(OldHeap, AccessExclusiveLock);
return;
--- 381,394 ----
check_index_is_clusterable(OldHeap, indexOid, recheck, AccessExclusiveLock);
/*
! * Quietly ignore the request if this is a materialized view which has not
! * been populated from its query. No harm is done because there is no data
! * to deal with, and we don't want to throw an error if this is part of a
! * multi-relation request -- for example, CLUSTER was run on the entire
! * database.
*/
if (OldHeap->rd_rel->relkind == RELKIND_MATVIEW &&
! !OldHeap->rd_ispopulated)
{
relation_close(OldHeap, AccessExclusiveLock);
return;
***************
*** 923,929 **** copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
if (OldHeap->rd_rel->relkind == RELKIND_MATVIEW)
/* Make sure the heap looks good even if no rows are written. */
! SetRelationIsScannable(NewHeap);
/*
* Scan through the OldHeap, either in OldIndex order or sequentially;
--- 924,930 ----
if (OldHeap->rd_rel->relkind == RELKIND_MATVIEW)
/* Make sure the heap looks good even if no rows are written. */
! SetMatViewToPopulated(NewHeap);
/*
* Scan through the OldHeap, either in OldIndex order or sequentially;
*** a/src/backend/commands/createas.c
--- b/src/backend/commands/createas.c
***************
*** 417,423 **** intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
if (into->relkind == RELKIND_MATVIEW && !into->skipData)
/* Make sure the heap looks good even if no rows are written. */
! SetRelationIsScannable(intoRelationDesc);
/*
* Check INSERT permission on the constructed table.
--- 417,423 ----
if (into->relkind == RELKIND_MATVIEW && !into->skipData)
/* Make sure the heap looks good even if no rows are written. */
! SetMatViewToPopulated(intoRelationDesc);
/*
* Check INSERT permission on the constructed table.
*** a/src/backend/commands/matview.c
--- b/src/backend/commands/matview.c
***************
*** 52,73 **** static void refresh_matview_datafill(DestReceiver *dest, Query *query,
const char *queryString);
/*
! * SetRelationIsScannable
! * Make the relation appear scannable.
*
! * NOTE: This is only implemented for materialized views. The heap starts out
! * in a state that doesn't look scannable, and can only transition from there
! * to scannable, unless a new heap is created.
*
* NOTE: caller must be holding an appropriate lock on the relation.
*/
void
! SetRelationIsScannable(Relation relation)
{
Page page;
Assert(relation->rd_rel->relkind == RELKIND_MATVIEW);
! Assert(relation->rd_isscannable == false);
page = (Page) palloc(BLCKSZ);
PageInit(page, BLCKSZ, 0);
--- 52,72 ----
const char *queryString);
/*
! * SetMatViewToPopulated
! * Indicate that the materialized view has been populated by its query.
*
! * NOTE: The heap starts out in a state that doesn't look scannable, and can
! * only transition from there to scannable at the time a new heap is created.
*
* NOTE: caller must be holding an appropriate lock on the relation.
*/
void
! SetMatViewToPopulated(Relation relation)
{
Page page;
Assert(relation->rd_rel->relkind == RELKIND_MATVIEW);
! Assert(relation->rd_ispopulated == false);
page = (Page) palloc(BLCKSZ);
PageInit(page, BLCKSZ, 0);
***************
*** 323,329 **** transientrel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
myState->hi_options |= HEAP_INSERT_SKIP_WAL;
myState->bistate = GetBulkInsertState();
! SetRelationIsScannable(transientrel);
/* Not using WAL requires smgr_targblock be initially invalid */
Assert(RelationGetTargetBlock(transientrel) == InvalidBlockNumber);
--- 322,328 ----
myState->hi_options |= HEAP_INSERT_SKIP_WAL;
myState->bistate = GetBulkInsertState();
! SetMatViewToPopulated(transientrel);
/* Not using WAL requires smgr_targblock be initially invalid */
Assert(RelationGetTargetBlock(transientrel) == InvalidBlockNumber);
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***************
*** 499,505 **** ExecutorRewind(QueryDesc *queryDesc)
* Check that relations which are to be accessed are in a scannable
* state.
*
! * If not, throw error. For a materialized view, suggest refresh.
*/
static void
ExecCheckRelationsScannable(List *rangeTable)
--- 499,506 ----
* Check that relations which are to be accessed are in a scannable
* state.
*
! * Currently the only relations which are not are materialized views which
! * have not been populated by their queries.
*/
static void
ExecCheckRelationsScannable(List *rangeTable)
***************
*** 513,544 **** ExecCheckRelationsScannable(List *rangeTable)
if (rte->rtekind != RTE_RELATION)
continue;
! if (!RelationIdIsScannable(rte->relid))
! {
! if (rte->relkind == RELKIND_MATVIEW)
! {
! /* It is OK to replace the contents of an invalid matview. */
! if (rte->isResultRel)
! continue;
! ereport(ERROR,
! (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
! errmsg("materialized view \"%s\" has not been populated",
! get_rel_name(rte->relid)),
! errhint("Use the REFRESH MATERIALIZED VIEW command.")));
! }
! else
! /* This should never happen, so elog will do. */
! elog(ERROR, "relation \"%s\" is not flagged as scannable",
! get_rel_name(rte->relid));
! }
}
}
/*
! * Tells whether a relation is scannable.
*
! * Currently only non-populated materialzed views are not.
*/
static bool
RelationIdIsScannable(Oid relid)
--- 514,542 ----
if (rte->rtekind != RTE_RELATION)
continue;
! if (rte->relkind != RELKIND_MATVIEW)
! continue;
! /* It is OK to target an unpopulated materialized for results. */
! if (rte->isResultRel)
! continue;
!
! if (!RelationIdIsScannable(rte->relid))
! ereport(ERROR,
! (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
! errmsg("materialized view \"%s\" has not been populated",
! get_rel_name(rte->relid)),
! errhint("Use the REFRESH MATERIALIZED VIEW command.")));
}
}
/*
! * Tells whether a relation is scannable based on its OID.
! *
! * Currently only non-populated materialized views are not. This is likely to
! * change to include other conditions.
*
! * This should only be called while a lock is held on the relation.
*/
static bool
RelationIdIsScannable(Oid relid)
***************
*** 546,554 **** RelationIdIsScannable(Oid relid)
Relation relation;
bool result;
! relation = RelationIdGetRelation(relid);
! result = relation->rd_isscannable;
! RelationClose(relation);
return result;
}
--- 544,552 ----
Relation relation;
bool result;
! relation = heap_open(relid, NoLock);
! result = RelationIsScannable(relation);
! heap_close(relation, NoLock);
return result;
}
***************
*** 945,951 **** InitPlan(QueryDesc *queryDesc, int eflags)
/*
* Unless we are creating a view or are creating a materialized view WITH
! * NO DATA, ensure that all referenced relations are scannable.
*/
if ((eflags & EXEC_FLAG_WITH_NO_DATA) == 0)
ExecCheckRelationsScannable(rangeTable);
--- 943,956 ----
/*
* Unless we are creating a view or are creating a materialized view WITH
! * NO DATA, ensure that all referenced relations are scannable. The
! * omitted cases will be checked as SELECT statements in a different
! * phase, so checking again here would be wasteful and it would generate
! * errors on a materialized view referenced as a target.
! *
! * NB: This is being done after all relations are locked, files have been
! * opened, etc., to avoid duplicating that effort or creating deadlock
! * possibilities.
*/
if ((eflags & EXEC_FLAG_WITH_NO_DATA) == 0)
ExecCheckRelationsScannable(rangeTable);
*** a/src/backend/rewrite/rewriteHandler.c
--- b/src/backend/rewrite/rewriteHandler.c
***************
*** 1615,1621 **** fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
* expansion doesn't give us a lot to work with, so we are trusting
* earlier validations to throw error if needed.
*/
! if (rel->rd_rel->relkind == RELKIND_MATVIEW && rel->rd_isscannable)
{
heap_close(rel, NoLock);
continue;
--- 1615,1622 ----
* expansion doesn't give us a lot to work with, so we are trusting
* earlier validations to throw error if needed.
*/
! if (rel->rd_rel->relkind == RELKIND_MATVIEW &&
! RelationIsScannable(rel))
{
heap_close(rel, NoLock);
continue;
*** a/src/backend/utils/adt/dbsize.c
--- b/src/backend/utils/adt/dbsize.c
***************
*** 840,846 **** pg_relation_filepath(PG_FUNCTION_ARGS)
* Indicate whether a relation is scannable.
*
* Currently, this is always true except for a materialized view which has not
! * been populated.
*/
Datum
pg_relation_is_scannable(PG_FUNCTION_ARGS)
--- 840,847 ----
* Indicate whether a relation is scannable.
*
* Currently, this is always true except for a materialized view which has not
! * been populated. It is expected that other conditions for allowing a
! * materialized view to be scanned will be added in later releases.
*/
Datum
pg_relation_is_scannable(PG_FUNCTION_ARGS)
***************
*** 850,858 **** pg_relation_is_scannable(PG_FUNCTION_ARGS)
bool result;
relid = PG_GETARG_OID(0);
! relation = RelationIdGetRelation(relid);
! result = relation->rd_isscannable;
! RelationClose(relation);
PG_RETURN_BOOL(result);
}
--- 851,863 ----
bool result;
relid = PG_GETARG_OID(0);
! relation = try_relation_open(relid, AccessShareLock);
+ if (relation == NULL)
+ PG_RETURN_BOOL(false);
+
+ result = RelationIsScannable(relation);
+
+ relation_close(relation, AccessShareLock);
PG_RETURN_BOOL(result);
}
*** a/src/backend/utils/cache/relcache.c
--- b/src/backend/utils/cache/relcache.c
***************
*** 958,966 **** RelationBuildDesc(Oid targetRelId, bool insertIt)
if (relation->rd_rel->relkind == RELKIND_MATVIEW &&
heap_is_matview_init_state(relation))
! relation->rd_isscannable = false;
else
! relation->rd_isscannable = true;
/*
* now we can free the memory allocated for pg_class_tuple
--- 958,966 ----
if (relation->rd_rel->relkind == RELKIND_MATVIEW &&
heap_is_matview_init_state(relation))
! relation->rd_ispopulated = false;
else
! relation->rd_ispopulated = true;
/*
* now we can free the memory allocated for pg_class_tuple
***************
*** 1531,1537 **** formrdesc(const char *relationName, Oid relationReltype,
* initialize physical addressing information for the relation
*/
RelationInitPhysicalAddr(relation);
! relation->rd_isscannable = true;
/*
* initialize the rel-has-index flag, using hardwired knowledge
--- 1531,1537 ----
* initialize physical addressing information for the relation
*/
RelationInitPhysicalAddr(relation);
! relation->rd_ispopulated = true;
/*
* initialize the rel-has-index flag, using hardwired knowledge
***************
*** 1756,1762 **** RelationReloadIndexInfo(Relation relation)
heap_freetuple(pg_class_tuple);
/* We must recalculate physical address in case it changed */
RelationInitPhysicalAddr(relation);
! relation->rd_isscannable = true;
/*
* For a non-system index, there are fields of the pg_index row that are
--- 1756,1762 ----
heap_freetuple(pg_class_tuple);
/* We must recalculate physical address in case it changed */
RelationInitPhysicalAddr(relation);
! relation->rd_ispopulated = true;
/*
* For a non-system index, there are fields of the pg_index row that are
***************
*** 1907,1915 **** RelationClearRelation(Relation relation, bool rebuild)
RelationInitPhysicalAddr(relation);
if (relation->rd_rel->relkind == RELKIND_MATVIEW &&
heap_is_matview_init_state(relation))
! relation->rd_isscannable = false;
else
! relation->rd_isscannable = true;
if (relation->rd_rel->relkind == RELKIND_INDEX)
{
--- 1907,1915 ----
RelationInitPhysicalAddr(relation);
if (relation->rd_rel->relkind == RELKIND_MATVIEW &&
heap_is_matview_init_state(relation))
! relation->rd_ispopulated = false;
else
! relation->rd_ispopulated = true;
if (relation->rd_rel->relkind == RELKIND_INDEX)
{
***************
*** 2700,2708 **** RelationBuildLocalRelation(const char *relname,
/* materialized view not initially scannable */
if (relkind == RELKIND_MATVIEW)
! rel->rd_isscannable = false;
else
! rel->rd_isscannable = true;
/*
* Okay to insert into the relcache hash tables.
--- 2700,2708 ----
/* materialized view not initially scannable */
if (relkind == RELKIND_MATVIEW)
! rel->rd_ispopulated = false;
else
! rel->rd_ispopulated = true;
/*
* Okay to insert into the relcache hash tables.
***************
*** 4450,4458 **** load_relcache_init_file(bool shared)
RelationInitPhysicalAddr(rel);
if (rel->rd_rel->relkind == RELKIND_MATVIEW &&
heap_is_matview_init_state(rel))
! rel->rd_isscannable = false;
else
! rel->rd_isscannable = true;
}
/*
--- 4450,4458 ----
RelationInitPhysicalAddr(rel);
if (rel->rd_rel->relkind == RELKIND_MATVIEW &&
heap_is_matview_init_state(rel))
! rel->rd_ispopulated = false;
else
! rel->rd_ispopulated = true;
}
/*
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
***************
*** 4264,4270 **** getTables(Archive *fout, int *numTables)
"c.relhasindex, c.relhasrules, c.relhasoids, "
"c.relfrozenxid, tc.oid AS toid, "
"tc.relfrozenxid AS tfrozenxid, "
! "c.relpersistence, pg_relation_is_scannable(c.oid) as isscannable, "
"c.relpages, "
"CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype, "
"d.refobjid AS owning_tab, "
--- 4264,4271 ----
"c.relhasindex, c.relhasrules, c.relhasoids, "
"c.relfrozenxid, tc.oid AS toid, "
"tc.relfrozenxid AS tfrozenxid, "
! "c.relpersistence, "
! "CASE WHEN c.relkind = '%c' THEN pg_relation_is_scannable(c.oid) ELSE 't'::bool END as isscannable, "
"c.relpages, "
"CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype, "
"d.refobjid AS owning_tab, "
***************
*** 4282,4287 **** getTables(Archive *fout, int *numTables)
--- 4283,4289 ----
"WHERE c.relkind in ('%c', '%c', '%c', '%c', '%c', '%c') "
"ORDER BY c.oid",
username_subquery,
+ RELKIND_MATVIEW,
RELKIND_SEQUENCE,
RELKIND_RELATION, RELKIND_SEQUENCE,
RELKIND_VIEW, RELKIND_COMPOSITE_TYPE,
*** a/src/include/commands/matview.h
--- b/src/include/commands/matview.h
***************
*** 20,26 ****
#include "utils/relcache.h"
! extern void SetRelationIsScannable(Relation relation);
extern void ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
ParamListInfo params, char *completionTag);
--- 20,26 ----
#include "utils/relcache.h"
! extern void SetMatViewToPopulated(Relation relation);
extern void ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
ParamListInfo params, char *completionTag);
*** a/src/include/utils/rel.h
--- b/src/include/utils/rel.h
***************
*** 83,89 **** typedef struct RelationData
BackendId rd_backend; /* owning backend id, if temporary relation */
bool rd_islocaltemp; /* rel is a temp rel of this session */
bool rd_isnailed; /* rel is nailed in cache */
! bool rd_isscannable; /* rel can be scanned */
bool rd_isvalid; /* relcache entry is valid */
char rd_indexvalid; /* state of rd_indexlist: 0 = not valid, 1 =
* valid, 2 = temporarily forced */
--- 83,89 ----
BackendId rd_backend; /* owning backend id, if temporary relation */
bool rd_islocaltemp; /* rel is a temp rel of this session */
bool rd_isnailed; /* rel is nailed in cache */
! bool rd_ispopulated; /* matview has query results */
bool rd_isvalid; /* relcache entry is valid */
char rd_indexvalid; /* state of rd_indexlist: 0 = not valid, 1 =
* valid, 2 = temporarily forced */
***************
*** 407,412 **** typedef struct StdRdOptions
--- 407,422 ----
((relation)->rd_rel->relpersistence == RELPERSISTENCE_TEMP && \
!(relation)->rd_islocaltemp)
+
+ /*
+ * RelationIsScannable
+ * Currently can only be false for a materialized view which has not been
+ * populated by its query. This is likely to get more complicated later,
+ * so use a macro which looks like a function.
+ */
+ #define RelationIsScannable(relation) ((relation)->rd_ispopulated)
+
+
/* routines in utils/cache/relcache.c */
extern void RelationIncrementReferenceCount(Relation rel);
extern void RelationDecrementReferenceCount(Relation rel);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
