Fixing some issues in matview.c's refresh-query construction - Mailing list pgsql-hackers
| From | Tom Lane |
|---|---|
| Subject | Fixing some issues in matview.c's refresh-query construction |
| Date | |
| Msg-id | 13836.1521413227@sss.pgh.pa.us Whole thread Raw |
| List | pgsql-hackers |
While looking at the performance problem Jeff Janes reported recently[1],
I noticed several other pre-existing deficiencies in the way that
refresh_by_match_merge() generates its query for constructing the diff
table:
1. It doesn't require the selected unique index(es) to be indimmediate.
Perhaps this isn't a bug, but I don't really see why: if they're
deferred constraints, then they might not represent conditions that
actually hold right now.
2. It doesn't pay attention to the particular equality semantics enforced
by a given index, but just assumes that they must be those of the column
datatype's default btree opclass. The datatype might not even *have* a
default btree opclass. We must instead look up, and use, the equality
operator associated with the index's actual opclass.
3. It doesn't check that the indexes are btrees. Even though there are no
other unique-capable index AMs today (at least not in core), I think it's
a good idea to insist on btrees. We couldn't be sure that the planner
could implement FULL JOIN with an operator that is not btree equality, nor
do we have a convention for identifying what is the equality operator for
an index opclass if it's not btree.
4. It's insufficiently careful to ensure that the parser will pick the
intended operator when parsing the query. It's not enough to use
OPERATOR(schema.op) notation; you have to also be careful to cast the
inputs if they're not already of the operator's input types. The correct
way to do this is the way that the ri_triggers.c code does it. (This
would have been a security bug before CVE-2018-1058.)
5. While not actually a bug, I didn't like the fact that the conditions
for an index being usable for the match merge were written out in two
places. That's too easy to break. I also didn't like the fact that
refresh_by_match_merge() would segfault if it came across an index on a
system column, such as the OID column. That's just latent, since matviews
can't have OID columns today, but we really ought to check that we are
considering only indexes on user columns.
The attached patch rectifies these problems. Rather than duplicating
ri_triggers.c's code for emitting a safe equality clause, I thought the
best way to proceed was to have ruleutils.c export it for use by the
other modules. That's where we have most query-deconstruction code,
so just exporting it from ri_triggers.c seemed a bit too historical.
Barring objections, I propose to back-patch this as a bug fix.
regards, tom lane
[1] https://postgr.es/m/CAMkU=1z-JoGymHneGHar1cru4F1XDfHqJDzxP_CtK5cL3DOfmg@mail.gmail.com
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index ab6a889..7159a27 100644
*** a/src/backend/commands/matview.c
--- b/src/backend/commands/matview.c
***************
*** 21,26 ****
--- 21,28 ----
#include "catalog/catalog.h"
#include "catalog/indexing.h"
#include "catalog/namespace.h"
+ #include "catalog/pg_am.h"
+ #include "catalog/pg_opclass.h"
#include "catalog/pg_operator.h"
#include "commands/cluster.h"
#include "commands/matview.h"
***************
*** 40,46 ****
#include "utils/rel.h"
#include "utils/snapmgr.h"
#include "utils/syscache.h"
- #include "utils/typcache.h"
typedef struct
--- 42,47 ----
*************** static void transientrel_shutdown(DestRe
*** 62,75 ****
static void transientrel_destroy(DestReceiver *self);
static uint64 refresh_matview_datafill(DestReceiver *dest, Query *query,
const char *queryString);
-
static char *make_temptable_name_n(char *tempname, int n);
- static void mv_GenerateOper(StringInfo buf, Oid opoid);
-
static void refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
int save_sec_context);
static void refresh_by_heap_swap(Oid matviewOid, Oid OIDNewHeap, char relpersistence);
!
static void OpenMatViewIncrementalMaintenance(void);
static void CloseMatViewIncrementalMaintenance(void);
--- 63,73 ----
static void transientrel_destroy(DestReceiver *self);
static uint64 refresh_matview_datafill(DestReceiver *dest, Query *query,
const char *queryString);
static char *make_temptable_name_n(char *tempname, int n);
static void refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
int save_sec_context);
static void refresh_by_heap_swap(Oid matviewOid, Oid OIDNewHeap, char relpersistence);
! static bool is_usable_unique_index(Relation indexRel);
static void OpenMatViewIncrementalMaintenance(void);
static void CloseMatViewIncrementalMaintenance(void);
*************** ExecRefreshMatView(RefreshMatViewStmt *s
*** 230,252 ****
{
Oid indexoid = lfirst_oid(indexoidscan);
Relation indexRel;
- Form_pg_index indexStruct;
indexRel = index_open(indexoid, AccessShareLock);
! indexStruct = indexRel->rd_index;
!
! if (indexStruct->indisunique &&
! IndexIsValid(indexStruct) &&
! RelationGetIndexExpressions(indexRel) == NIL &&
! RelationGetIndexPredicate(indexRel) == NIL &&
! indexStruct->indnatts > 0)
! {
! hasUniqueIndex = true;
! index_close(indexRel, AccessShareLock);
! break;
! }
!
index_close(indexRel, AccessShareLock);
}
list_free(indexoidlist);
--- 228,239 ----
{
Oid indexoid = lfirst_oid(indexoidscan);
Relation indexRel;
indexRel = index_open(indexoid, AccessShareLock);
! hasUniqueIndex = is_usable_unique_index(indexRel);
index_close(indexRel, AccessShareLock);
+ if (hasUniqueIndex)
+ break;
}
list_free(indexoidlist);
*************** make_temptable_name_n(char *tempname, in
*** 557,581 ****
return namebuf.data;
}
- static void
- mv_GenerateOper(StringInfo buf, Oid opoid)
- {
- HeapTuple opertup;
- Form_pg_operator operform;
-
- opertup = SearchSysCache1(OPEROID, ObjectIdGetDatum(opoid));
- if (!HeapTupleIsValid(opertup))
- elog(ERROR, "cache lookup failed for operator %u", opoid);
- operform = (Form_pg_operator) GETSTRUCT(opertup);
- Assert(operform->oprkind == 'b');
-
- appendStringInfo(buf, "OPERATOR(%s.%s)",
- quote_identifier(get_namespace_name(operform->oprnamespace)),
- NameStr(operform->oprname));
-
- ReleaseSysCache(opertup);
- }
-
/*
* refresh_by_match_merge
*
--- 544,549 ----
*************** refresh_by_match_merge(Oid matviewOid, O
*** 623,629 ****
List *indexoidlist;
ListCell *indexoidscan;
int16 relnatts;
! bool *usedForQual;
initStringInfo(&querybuf);
matviewRel = heap_open(matviewOid, NoLock);
--- 591,597 ----
List *indexoidlist;
ListCell *indexoidscan;
int16 relnatts;
! Oid *opUsedForQual;
initStringInfo(&querybuf);
matviewRel = heap_open(matviewOid, NoLock);
*************** refresh_by_match_merge(Oid matviewOid, O
*** 635,641 ****
diffname = make_temptable_name_n(tempname, 2);
relnatts = matviewRel->rd_rel->relnatts;
- usedForQual = (bool *) palloc0(sizeof(bool) * relnatts);
/* Open SPI context. */
if (SPI_connect() != SPI_OK_CONNECT)
--- 603,608 ----
*************** refresh_by_match_merge(Oid matviewOid, O
*** 699,744 ****
* include all rows.
*/
tupdesc = matviewRel->rd_att;
foundUniqueIndex = false;
indexoidlist = RelationGetIndexList(matviewRel);
foreach(indexoidscan, indexoidlist)
{
Oid indexoid = lfirst_oid(indexoidscan);
Relation indexRel;
- Form_pg_index indexStruct;
indexRel = index_open(indexoid, RowExclusiveLock);
! indexStruct = indexRel->rd_index;
!
! /*
! * We're only interested if it is unique, valid, contains no
! * expressions, and is not partial.
! */
! if (indexStruct->indisunique &&
! IndexIsValid(indexStruct) &&
! RelationGetIndexExpressions(indexRel) == NIL &&
! RelationGetIndexPredicate(indexRel) == NIL)
{
int numatts = indexStruct->indnatts;
int i;
/* Add quals for all columns from this index. */
for (i = 0; i < numatts; i++)
{
int attnum = indexStruct->indkey.values[i];
Form_pg_attribute attr = TupleDescAttr(tupdesc, attnum - 1);
! Oid type;
Oid op;
! const char *colname;
/*
! * Only include the column once regardless of how many times
! * it shows up in how many indexes.
*/
! if (usedForQual[attnum - 1])
continue;
! usedForQual[attnum - 1] = true;
/*
* Actually add the qual, ANDed with any others.
--- 666,747 ----
* include all rows.
*/
tupdesc = matviewRel->rd_att;
+ opUsedForQual = (Oid *) palloc0(sizeof(Oid) * relnatts);
foundUniqueIndex = false;
+
indexoidlist = RelationGetIndexList(matviewRel);
foreach(indexoidscan, indexoidlist)
{
Oid indexoid = lfirst_oid(indexoidscan);
Relation indexRel;
indexRel = index_open(indexoid, RowExclusiveLock);
! if (is_usable_unique_index(indexRel))
{
+ Form_pg_index indexStruct = indexRel->rd_index;
int numatts = indexStruct->indnatts;
+ oidvector *indclass;
+ Datum indclassDatum;
+ bool isnull;
int i;
+ /* Must get indclass the hard way. */
+ indclassDatum = SysCacheGetAttr(INDEXRELID,
+ indexRel->rd_indextuple,
+ Anum_pg_index_indclass,
+ &isnull);
+ Assert(!isnull);
+ indclass = (oidvector *) DatumGetPointer(indclassDatum);
+
/* Add quals for all columns from this index. */
for (i = 0; i < numatts; i++)
{
int attnum = indexStruct->indkey.values[i];
+ Oid opclass = indclass->values[i];
Form_pg_attribute attr = TupleDescAttr(tupdesc, attnum - 1);
! Oid attrtype = attr->atttypid;
! HeapTuple cla_ht;
! Form_pg_opclass cla_tup;
! Oid opfamily;
! Oid opcintype;
Oid op;
! const char *leftop;
! const char *rightop;
/*
! * Identify the equality operator associated with this index
! * column. First we need to look up the column's opclass.
*/
! cla_ht = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclass));
! if (!HeapTupleIsValid(cla_ht))
! elog(ERROR, "cache lookup failed for opclass %u", opclass);
! cla_tup = (Form_pg_opclass) GETSTRUCT(cla_ht);
! Assert(cla_tup->opcmethod == BTREE_AM_OID);
! opfamily = cla_tup->opcfamily;
! opcintype = cla_tup->opcintype;
! ReleaseSysCache(cla_ht);
!
! op = get_opfamily_member(opfamily, opcintype, opcintype,
! BTEqualStrategyNumber);
! if (!OidIsValid(op))
! elog(ERROR, "missing operator %d(%u,%u) in opfamily %u",
! BTEqualStrategyNumber, opcintype, opcintype, opfamily);
!
! /*
! * If we find the same column with the same equality semantics
! * in more than one index, we only need to emit the equality
! * clause once.
! *
! * Since we only remember the last equality operator, this
! * code could be fooled into emitting duplicate clauses given
! * multiple indexes with several different opclasses ... but
! * that's so unlikely it doesn't seem worth spending extra
! * code to avoid.
! */
! if (opUsedForQual[attnum - 1] == op)
continue;
! opUsedForQual[attnum - 1] = op;
/*
* Actually add the qual, ANDed with any others.
*************** refresh_by_match_merge(Oid matviewOid, O
*** 746,757 ****
if (foundUniqueIndex)
appendStringInfoString(&querybuf, " AND ");
! colname = quote_identifier(NameStr(attr->attname));
! appendStringInfo(&querybuf, "newdata.%s ", colname);
! type = attnumTypeId(matviewRel, attnum);
! op = lookup_type_cache(type, TYPECACHE_EQ_OPR)->eq_opr;
! mv_GenerateOper(&querybuf, op);
! appendStringInfo(&querybuf, " mv.%s", colname);
foundUniqueIndex = true;
}
--- 749,763 ----
if (foundUniqueIndex)
appendStringInfoString(&querybuf, " AND ");
! leftop = quote_qualified_identifier("newdata",
! NameStr(attr->attname));
! rightop = quote_qualified_identifier("mv",
! NameStr(attr->attname));
!
! generate_operator_clause(&querybuf,
! leftop, attrtype,
! op,
! rightop, attrtype);
foundUniqueIndex = true;
}
*************** refresh_by_match_merge(Oid matviewOid, O
*** 764,774 ****
list_free(indexoidlist);
/*
! * There must be at least one unique index on the matview.
*
* ExecRefreshMatView() checks that after taking the exclusive lock on the
* matview. So at least one unique index is guaranteed to exist here
! * because the lock is still being held.
*/
Assert(foundUniqueIndex);
--- 770,780 ----
list_free(indexoidlist);
/*
! * There must be at least one usable unique index on the matview.
*
* ExecRefreshMatView() checks that after taking the exclusive lock on the
* matview. So at least one unique index is guaranteed to exist here
! * because the lock is still being held; so an Assert seems sufficient.
*/
Assert(foundUniqueIndex);
*************** refresh_by_heap_swap(Oid matviewOid, Oid
*** 845,850 ****
--- 851,901 ----
RecentXmin, ReadNextMultiXactId(), relpersistence);
}
+ /*
+ * Check whether specified index is usable for match merge.
+ */
+ static bool
+ is_usable_unique_index(Relation indexRel)
+ {
+ Form_pg_index indexStruct = indexRel->rd_index;
+
+ /*
+ * Must be unique, valid, immediate, non-partial, and be defined over
+ * plain user columns (not expressions). We also require it to be a
+ * btree. Even if we had any other unique index kinds, we'd not know how
+ * to identify the corresponding equality operator, nor could we be sure
+ * that the planner could implement the required FULL JOIN with non-btree
+ * operators.
+ */
+ if (indexStruct->indisunique &&
+ indexStruct->indimmediate &&
+ indexRel->rd_rel->relam == BTREE_AM_OID &&
+ IndexIsValid(indexStruct) &&
+ RelationGetIndexPredicate(indexRel) == NIL &&
+ indexStruct->indnatts > 0)
+ {
+ /*
+ * The point of groveling through the index columns individually is to
+ * reject both index expressions and system columns. Currently,
+ * matviews couldn't have OID columns so there's no way to create an
+ * index on a system column; but maybe someday that wouldn't be true,
+ * so let's be safe.
+ */
+ int numatts = indexStruct->indnatts;
+ int i;
+
+ for (i = 0; i < numatts; i++)
+ {
+ int attnum = indexStruct->indkey.values[i];
+
+ if (attnum <= 0)
+ return false;
+ }
+ return true;
+ }
+ return false;
+ }
+
/*
* This should be used to test whether the backend is in a context where it is
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 8faae1d..4d7fee0 100644
*** a/src/backend/utils/adt/ri_triggers.c
--- b/src/backend/utils/adt/ri_triggers.c
*************** static void ri_GenerateQual(StringInfo b
*** 203,209 ****
const char *leftop, Oid leftoptype,
Oid opoid,
const char *rightop, Oid rightoptype);
- static void ri_add_cast_to(StringInfo buf, Oid typid);
static void ri_GenerateQualCollation(StringInfo buf, Oid collation);
static int ri_NullCheck(HeapTuple tup,
const RI_ConstraintInfo *riinfo, bool rel_is_pk);
--- 203,208 ----
*************** quoteRelationName(char *buffer, Relation
*** 2134,2146 ****
/*
* ri_GenerateQual --- generate a WHERE clause equating two variables
*
! * The idea is to append " sep leftop op rightop" to buf. The complexity
! * comes from needing to be sure that the parser will select the desired
! * operator. We always name the operator using OPERATOR(schema.op) syntax
! * (readability isn't a big priority here), so as to avoid search-path
! * uncertainties. We have to emit casts too, if either input isn't already
! * the input type of the operator; else we are at the mercy of the parser's
! * heuristics for ambiguous-operator resolution.
*/
static void
ri_GenerateQual(StringInfo buf,
--- 2133,2142 ----
/*
* ri_GenerateQual --- generate a WHERE clause equating two variables
*
! * This basically appends " sep leftop op rightop" to buf, adding casts
! * and schema qualification as needed to ensure that the parser will select
! * the operator we specify. leftop and rightop should be parenthesized
! * if they aren't variables or parameters.
*/
static void
ri_GenerateQual(StringInfo buf,
*************** ri_GenerateQual(StringInfo buf,
*** 2149,2208 ****
Oid opoid,
const char *rightop, Oid rightoptype)
{
! HeapTuple opertup;
! Form_pg_operator operform;
! char *oprname;
! char *nspname;
!
! opertup = SearchSysCache1(OPEROID, ObjectIdGetDatum(opoid));
! if (!HeapTupleIsValid(opertup))
! elog(ERROR, "cache lookup failed for operator %u", opoid);
! operform = (Form_pg_operator) GETSTRUCT(opertup);
! Assert(operform->oprkind == 'b');
! oprname = NameStr(operform->oprname);
!
! nspname = get_namespace_name(operform->oprnamespace);
!
! appendStringInfo(buf, " %s %s", sep, leftop);
! if (leftoptype != operform->oprleft)
! ri_add_cast_to(buf, operform->oprleft);
! appendStringInfo(buf, " OPERATOR(%s.", quote_identifier(nspname));
! appendStringInfoString(buf, oprname);
! appendStringInfo(buf, ") %s", rightop);
! if (rightoptype != operform->oprright)
! ri_add_cast_to(buf, operform->oprright);
!
! ReleaseSysCache(opertup);
! }
!
! /*
! * Add a cast specification to buf. We spell out the type name the hard way,
! * intentionally not using format_type_be(). This is to avoid corner cases
! * for CHARACTER, BIT, and perhaps other types, where specifying the type
! * using SQL-standard syntax results in undesirable data truncation. By
! * doing it this way we can be certain that the cast will have default (-1)
! * target typmod.
! */
! static void
! ri_add_cast_to(StringInfo buf, Oid typid)
! {
! HeapTuple typetup;
! Form_pg_type typform;
! char *typname;
! char *nspname;
!
! typetup = SearchSysCache1(TYPEOID, ObjectIdGetDatum(typid));
! if (!HeapTupleIsValid(typetup))
! elog(ERROR, "cache lookup failed for type %u", typid);
! typform = (Form_pg_type) GETSTRUCT(typetup);
!
! typname = NameStr(typform->typname);
! nspname = get_namespace_name(typform->typnamespace);
!
! appendStringInfo(buf, "::%s.%s",
! quote_identifier(nspname), quote_identifier(typname));
!
! ReleaseSysCache(typetup);
}
/*
--- 2145,2153 ----
Oid opoid,
const char *rightop, Oid rightoptype)
{
! appendStringInfo(buf, " %s ", sep);
! generate_operator_clause(buf, leftop, leftoptype, opoid,
! rightop, rightoptype);
}
/*
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index b58ee3c..2cd54ec 100644
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
*************** static char *generate_function_name(Oid
*** 462,467 ****
--- 462,468 ----
bool has_variadic, bool *use_variadic_p,
ParseExprKind special_exprkind);
static char *generate_operator_name(Oid operid, Oid arg1, Oid arg2);
+ static void add_cast_to(StringInfo buf, Oid typid);
static char *generate_qualified_type_name(Oid typid);
static text *string_to_text(char *str);
static char *flatten_reloptions(Oid relid);
*************** generate_operator_name(Oid operid, Oid a
*** 10850,10855 ****
--- 10851,10935 ----
}
/*
+ * generate_operator_clause --- generate a binary-operator WHERE clause
+ *
+ * This is used for internally-generated-and-executed SQL queries, where
+ * precision is essential and readability is secondary. The basic
+ * requirement is to append "leftop op rightop" to buf, where leftop and
+ * rightop are given as strings and are assumed to yield types leftoptype
+ * and rightoptype; the operator is identified by OID. The complexity
+ * comes from needing to be sure that the parser will select the desired
+ * operator when the query is parsed. We always name the operator using
+ * OPERATOR(schema.op) syntax, so as to avoid search-path uncertainties.
+ * We have to emit casts too, if either input isn't already the input type
+ * of the operator; else we are at the mercy of the parser's heuristics for
+ * ambiguous-operator resolution. The caller must ensure that leftop and
+ * rightop are suitable arguments for a cast operation; it's best to insert
+ * parentheses if they aren't just variables or parameters.
+ */
+ void
+ generate_operator_clause(StringInfo buf,
+ const char *leftop, Oid leftoptype,
+ Oid opoid,
+ const char *rightop, Oid rightoptype)
+ {
+ HeapTuple opertup;
+ Form_pg_operator operform;
+ char *oprname;
+ char *nspname;
+
+ opertup = SearchSysCache1(OPEROID, ObjectIdGetDatum(opoid));
+ if (!HeapTupleIsValid(opertup))
+ elog(ERROR, "cache lookup failed for operator %u", opoid);
+ operform = (Form_pg_operator) GETSTRUCT(opertup);
+ Assert(operform->oprkind == 'b');
+ oprname = NameStr(operform->oprname);
+
+ nspname = get_namespace_name(operform->oprnamespace);
+
+ appendStringInfoString(buf, leftop);
+ if (leftoptype != operform->oprleft)
+ add_cast_to(buf, operform->oprleft);
+ appendStringInfo(buf, " OPERATOR(%s.", quote_identifier(nspname));
+ appendStringInfoString(buf, oprname);
+ appendStringInfo(buf, ") %s", rightop);
+ if (rightoptype != operform->oprright)
+ add_cast_to(buf, operform->oprright);
+
+ ReleaseSysCache(opertup);
+ }
+
+ /*
+ * Add a cast specification to buf. We spell out the type name the hard way,
+ * intentionally not using format_type_be(). This is to avoid corner cases
+ * for CHARACTER, BIT, and perhaps other types, where specifying the type
+ * using SQL-standard syntax results in undesirable data truncation. By
+ * doing it this way we can be certain that the cast will have default (-1)
+ * target typmod.
+ */
+ static void
+ add_cast_to(StringInfo buf, Oid typid)
+ {
+ HeapTuple typetup;
+ Form_pg_type typform;
+ char *typname;
+ char *nspname;
+
+ typetup = SearchSysCache1(TYPEOID, ObjectIdGetDatum(typid));
+ if (!HeapTupleIsValid(typetup))
+ elog(ERROR, "cache lookup failed for type %u", typid);
+ typform = (Form_pg_type) GETSTRUCT(typetup);
+
+ typname = NameStr(typform->typname);
+ nspname = get_namespace_name(typform->typnamespace);
+
+ appendStringInfo(buf, "::%s.%s",
+ quote_identifier(nspname), quote_identifier(typname));
+
+ ReleaseSysCache(typetup);
+ }
+
+ /*
* generate_qualified_type_name
* Compute the name to display for a type specified by OID
*
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index 01e76ae..d0416e9 100644
*** a/src/include/utils/builtins.h
--- b/src/include/utils/builtins.h
*************** extern bool quote_all_identifiers;
*** 78,83 ****
--- 78,87 ----
extern const char *quote_identifier(const char *ident);
extern char *quote_qualified_identifier(const char *qualifier,
const char *ident);
+ extern void generate_operator_clause(fmStringInfo buf,
+ const char *leftop, Oid leftoptype,
+ Oid opoid,
+ const char *rightop, Oid rightoptype);
/* varchar.c */
extern int bpchartruelen(char *s, int len);
pgsql-hackers by date: