Skip to content

Commit 7d7b59a

Browse files
tglsfdctglsfdc
committed
Fix some corner-case issues in REFRESH MATERIALIZED VIEW CONCURRENTLY.
refresh_by_match_merge() has some issues in the way it builds a SQL query to construct the "diff" table: 1. It doesn't require the selected unique index(es) to be indimmediate. 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. 3. It doesn't check that the indexes are btrees. 4. It's insufficiently careful to ensure that the parser will pick the intended operator when parsing the query. (This would have been a security bug before CVE-2018-1058.) 5. It's not careful about indexes on system columns. The way to fix #4 is to make use of the existing code in ri_triggers.c for generating an arbitrary binary operator clause. I chose to move that to ruleutils.c, since that seems a more reasonable place to be exporting such functionality from than ri_triggers.c. While #1, #3, and #5 are just latent given existing feature restrictions, and #2 doesn't arise in the core system for lack of alternate opclasses with different equality behaviors, #4 seems like an issue worth back-patching. That's the bulk of the change anyway, so just back-patch the whole thing to 9.4 where this code was introduced. Discussion: https://postgr.es/m/13836.1521413227@sss.pgh.pa.us
1 parent ebcf34d commit 7d7b59a

File tree

4 files changed

+208
-127
lines changed

4 files changed

+208
-127
lines changed

‎src/backend/commands/matview.c‎

Lines changed: 117 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121
#include "catalog/catalog.h"
2222
#include "catalog/indexing.h"
2323
#include "catalog/namespace.h"
24+
#include "catalog/pg_am.h"
25+
#include "catalog/pg_opclass.h"
2426
#include "catalog/pg_operator.h"
2527
#include "commands/cluster.h"
2628
#include "commands/matview.h"
@@ -39,7 +41,6 @@
3941
#include "utils/rel.h"
4042
#include "utils/snapmgr.h"
4143
#include "utils/syscache.h"
42-
#include "utils/typcache.h"
4344

4445

4546
typedef struct
@@ -61,14 +62,11 @@ static void transientrel_shutdown(DestReceiver *self);
6162
static void transientrel_destroy(DestReceiver *self);
6263
static void refresh_matview_datafill(DestReceiver *dest, Query *query,
6364
const char *queryString);
64-
6565
static char *make_temptable_name_n(char *tempname, int n);
66-
static void mv_GenerateOper(StringInfo buf, Oid opoid);
67-
6866
static void refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
6967
int save_sec_context);
7068
static void refresh_by_heap_swap(Oid matviewOid, Oid OIDNewHeap, char relpersistence);
71-
69+
static bool is_usable_unique_index(Relation indexRel);
7270
static void OpenMatViewIncrementalMaintenance(void);
7371
static void CloseMatViewIncrementalMaintenance(void);
7472

@@ -230,23 +228,12 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
230228
{
231229
Oid indexoid = lfirst_oid(indexoidscan);
232230
Relation indexRel;
233-
Form_pg_index indexStruct;
234231

235232
indexRel = index_open(indexoid, AccessShareLock);
236-
indexStruct = indexRel->rd_index;
237-
238-
if (indexStruct->indisunique &&
239-
IndexIsValid(indexStruct) &&
240-
RelationGetIndexExpressions(indexRel) == NIL &&
241-
RelationGetIndexPredicate(indexRel) == NIL &&
242-
indexStruct->indnatts > 0)
243-
{
244-
hasUniqueIndex = true;
245-
index_close(indexRel, AccessShareLock);
246-
break;
247-
}
248-
233+
hasUniqueIndex = is_usable_unique_index(indexRel);
249234
index_close(indexRel, AccessShareLock);
235+
if (hasUniqueIndex)
236+
break;
250237
}
251238

252239
list_free(indexoidlist);
@@ -536,25 +523,6 @@ make_temptable_name_n(char *tempname, int n)
536523
return namebuf.data;
537524
}
538525

539-
static void
540-
mv_GenerateOper(StringInfo buf, Oid opoid)
541-
{
542-
HeapTuple opertup;
543-
Form_pg_operator operform;
544-
545-
opertup = SearchSysCache1(OPEROID, ObjectIdGetDatum(opoid));
546-
if (!HeapTupleIsValid(opertup))
547-
elog(ERROR, "cache lookup failed for operator %u", opoid);
548-
operform = (Form_pg_operator) GETSTRUCT(opertup);
549-
Assert(operform->oprkind == 'b');
550-
551-
appendStringInfo(buf, "OPERATOR(%s.%s)",
552-
quote_identifier(get_namespace_name(operform->oprnamespace)),
553-
NameStr(operform->oprname));
554-
555-
ReleaseSysCache(opertup);
556-
}
557-
558526
/*
559527
* refresh_by_match_merge
560528
*
@@ -602,7 +570,7 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
602570
List *indexoidlist;
603571
ListCell *indexoidscan;
604572
int16 relnatts;
605-
bool *usedForQual;
573+
Oid *opUsedForQual;
606574

607575
initStringInfo(&querybuf);
608576
matviewRel = heap_open(matviewOid, NoLock);
@@ -614,7 +582,6 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
614582
diffname = make_temptable_name_n(tempname, 2);
615583

616584
relnatts = matviewRel->rd_rel->relnatts;
617-
usedForQual = (bool *) palloc0(sizeof(bool) * relnatts);
618585

619586
/* Open SPI context. */
620587
if (SPI_connect() != SPI_OK_CONNECT)
@@ -678,58 +645,98 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
678645
* include all rows.
679646
*/
680647
tupdesc = matviewRel->rd_att;
648+
opUsedForQual = (Oid *) palloc0(sizeof(Oid) * relnatts);
681649
foundUniqueIndex = false;
650+
682651
indexoidlist = RelationGetIndexList(matviewRel);
683652

684653
foreach(indexoidscan, indexoidlist)
685654
{
686655
Oid indexoid = lfirst_oid(indexoidscan);
687656
Relation indexRel;
688-
Form_pg_index indexStruct;
689657

690658
indexRel = index_open(indexoid, RowExclusiveLock);
691-
indexStruct = indexRel->rd_index;
692-
693-
/*
694-
* We're only interested if it is unique, valid, contains no
695-
* expressions, and is not partial.
696-
*/
697-
if (indexStruct->indisunique &&
698-
IndexIsValid(indexStruct) &&
699-
RelationGetIndexExpressions(indexRel) == NIL &&
700-
RelationGetIndexPredicate(indexRel) == NIL)
659+
if (is_usable_unique_index(indexRel))
701660
{
661+
Form_pg_index indexStruct = indexRel->rd_index;
702662
int numatts = indexStruct->indnatts;
663+
oidvector *indclass;
664+
Datum indclassDatum;
665+
bool isnull;
703666
int i;
704667

668+
/* Must get indclass the hard way. */
669+
indclassDatum = SysCacheGetAttr(INDEXRELID,
670+
indexRel->rd_indextuple,
671+
Anum_pg_index_indclass,
672+
&isnull);
673+
Assert(!isnull);
674+
indclass = (oidvector *) DatumGetPointer(indclassDatum);
675+
705676
/* Add quals for all columns from this index. */
706677
for (i = 0; i < numatts; i++)
707678
{
708679
int attnum = indexStruct->indkey.values[i];
709-
Oid type;
680+
Oid opclass = indclass->values[i];
681+
Form_pg_attribute attr = TupleDescAttr(tupdesc, attnum - 1);
682+
Oid attrtype = attr->atttypid;
683+
HeapTuple cla_ht;
684+
Form_pg_opclass cla_tup;
685+
Oid opfamily;
686+
Oid opcintype;
710687
Oid op;
711-
const char *colname;
688+
const char *leftop;
689+
const char *rightop;
712690

713691
/*
714-
* Only include the column once regardless of how many times
715-
* it shows up in how many indexes.
692+
* Identify the equality operator associated with this index
693+
* column. First we need to look up the column's opclass.
716694
*/
717-
if (usedForQual[attnum - 1])
695+
cla_ht = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclass));
696+
if (!HeapTupleIsValid(cla_ht))
697+
elog(ERROR, "cache lookup failed for opclass %u", opclass);
698+
cla_tup = (Form_pg_opclass) GETSTRUCT(cla_ht);
699+
Assert(cla_tup->opcmethod == BTREE_AM_OID);
700+
opfamily = cla_tup->opcfamily;
701+
opcintype = cla_tup->opcintype;
702+
ReleaseSysCache(cla_ht);
703+
704+
op = get_opfamily_member(opfamily, opcintype, opcintype,
705+
BTEqualStrategyNumber);
706+
if (!OidIsValid(op))
707+
elog(ERROR, "missing operator %d(%u,%u) in opfamily %u",
708+
BTEqualStrategyNumber, opcintype, opcintype, opfamily);
709+
710+
/*
711+
* If we find the same column with the same equality semantics
712+
* in more than one index, we only need to emit the equality
713+
* clause once.
714+
*
715+
* Since we only remember the last equality operator, this
716+
* code could be fooled into emitting duplicate clauses given
717+
* multiple indexes with several different opclasses ... but
718+
* that's so unlikely it doesn't seem worth spending extra
719+
* code to avoid.
720+
*/
721+
if (opUsedForQual[attnum - 1] == op)
718722
continue;
719-
usedForQual[attnum - 1] = true;
723+
opUsedForQual[attnum - 1] = op;
720724

721725
/*
722726
* Actually add the qual, ANDed with any others.
723727
*/
724728
if (foundUniqueIndex)
725729
appendStringInfoString(&querybuf, " AND ");
726730

727-
colname = quote_identifier(NameStr((tupdesc->attrs[attnum - 1])->attname));
728-
appendStringInfo(&querybuf, "newdata.%s ", colname);
729-
type = attnumTypeId(matviewRel, attnum);
730-
op = lookup_type_cache(type, TYPECACHE_EQ_OPR)->eq_opr;
731-
mv_GenerateOper(&querybuf, op);
732-
appendStringInfo(&querybuf, " mv.%s", colname);
731+
leftop = quote_qualified_identifier("newdata",
732+
NameStr(attr->attname));
733+
rightop = quote_qualified_identifier("mv",
734+
NameStr(attr->attname));
735+
736+
generate_operator_clause(&querybuf,
737+
leftop, attrtype,
738+
op,
739+
rightop, attrtype);
733740

734741
foundUniqueIndex = true;
735742
}
@@ -742,11 +749,11 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
742749
list_free(indexoidlist);
743750

744751
/*
745-
* There must be at least one unique index on the matview.
752+
* There must be at least one usable unique index on the matview.
746753
*
747754
* ExecRefreshMatView() checks that after taking the exclusive lock on the
748755
* matview. So at least one unique index is guaranteed to exist here
749-
* because the lock is still being held.
756+
* because the lock is still being held; so an Assert seems sufficient.
750757
*/
751758
Assert(foundUniqueIndex);
752759

@@ -823,6 +830,51 @@ refresh_by_heap_swap(Oid matviewOid, Oid OIDNewHeap, char relpersistence)
823830
RecentXmin, ReadNextMultiXactId(), relpersistence);
824831
}
825832

833+
/*
834+
* Check whether specified index is usable for match merge.
835+
*/
836+
static bool
837+
is_usable_unique_index(Relation indexRel)
838+
{
839+
Form_pg_index indexStruct = indexRel->rd_index;
840+
841+
/*
842+
* Must be unique, valid, immediate, non-partial, and be defined over
843+
* plain user columns (not expressions). We also require it to be a
844+
* btree. Even if we had any other unique index kinds, we'd not know how
845+
* to identify the corresponding equality operator, nor could we be sure
846+
* that the planner could implement the required FULL JOIN with non-btree
847+
* operators.
848+
*/
849+
if (indexStruct->indisunique &&
850+
indexStruct->indimmediate &&
851+
indexRel->rd_rel->relam == BTREE_AM_OID &&
852+
IndexIsValid(indexStruct) &&
853+
RelationGetIndexPredicate(indexRel) == NIL &&
854+
indexStruct->indnatts > 0)
855+
{
856+
/*
857+
* The point of groveling through the index columns individually is to
858+
* reject both index expressions and system columns. Currently,
859+
* matviews couldn't have OID columns so there's no way to create an
860+
* index on a system column; but maybe someday that wouldn't be true,
861+
* so let's be safe.
862+
*/
863+
int numatts = indexStruct->indnatts;
864+
int i;
865+
866+
for (i = 0; i < numatts; i++)
867+
{
868+
int attnum = indexStruct->indkey.values[i];
869+
870+
if (attnum <= 0)
871+
return false;
872+
}
873+
return true;
874+
}
875+
return false;
876+
}
877+
826878

827879
/*
828880
* This should be used to test whether the backend is in a context where it is

‎src/backend/utils/adt/ri_triggers.c‎

Lines changed: 7 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,6 @@ static void ri_GenerateQual(StringInfo buf,
208208
const char *leftop, Oid leftoptype,
209209
Oid opoid,
210210
const char *rightop, Oid rightoptype);
211-
static void ri_add_cast_to(StringInfo buf, Oid typid);
212211
static void ri_GenerateQualCollation(StringInfo buf, Oid collation);
213212
static int ri_NullCheck(HeapTuple tup,
214213
const RI_ConstraintInfo *riinfo, bool rel_is_pk);
@@ -2560,13 +2559,10 @@ quoteRelationName(char *buffer, Relation rel)
25602559
/*
25612560
* ri_GenerateQual --- generate a WHERE clause equating two variables
25622561
*
2563-
* The idea is to append " sep leftop op rightop" to buf. The complexity
2564-
* comes from needing to be sure that the parser will select the desired
2565-
* operator. We always name the operator using OPERATOR(schema.op) syntax
2566-
* (readability isn't a big priority here), so as to avoid search-path
2567-
* uncertainties. We have to emit casts too, if either input isn't already
2568-
* the input type of the operator; else we are at the mercy of the parser's
2569-
* heuristics for ambiguous-operator resolution.
2562+
* This basically appends " sep leftop op rightop" to buf, adding casts
2563+
* and schema qualification as needed to ensure that the parser will select
2564+
* the operator we specify. leftop and rightop should be parenthesized
2565+
* if they aren't variables or parameters.
25702566
*/
25712567
static void
25722568
ri_GenerateQual(StringInfo buf,
@@ -2575,60 +2571,9 @@ ri_GenerateQual(StringInfo buf,
25752571
Oid opoid,
25762572
const char *rightop, Oid rightoptype)
25772573
{
2578-
HeapTuple opertup;
2579-
Form_pg_operator operform;
2580-
char *oprname;
2581-
char *nspname;
2582-
2583-
opertup = SearchSysCache1(OPEROID, ObjectIdGetDatum(opoid));
2584-
if (!HeapTupleIsValid(opertup))
2585-
elog(ERROR, "cache lookup failed for operator %u", opoid);
2586-
operform = (Form_pg_operator) GETSTRUCT(opertup);
2587-
Assert(operform->oprkind == 'b');
2588-
oprname = NameStr(operform->oprname);
2589-
2590-
nspname = get_namespace_name(operform->oprnamespace);
2591-
2592-
appendStringInfo(buf, " %s %s", sep, leftop);
2593-
if (leftoptype != operform->oprleft)
2594-
ri_add_cast_to(buf, operform->oprleft);
2595-
appendStringInfo(buf, " OPERATOR(%s.", quote_identifier(nspname));
2596-
appendStringInfoString(buf, oprname);
2597-
appendStringInfo(buf, ") %s", rightop);
2598-
if (rightoptype != operform->oprright)
2599-
ri_add_cast_to(buf, operform->oprright);
2600-
2601-
ReleaseSysCache(opertup);
2602-
}
2603-
2604-
/*
2605-
* Add a cast specification to buf. We spell out the type name the hard way,
2606-
* intentionally not using format_type_be(). This is to avoid corner cases
2607-
* for CHARACTER, BIT, and perhaps other types, where specifying the type
2608-
* using SQL-standard syntax results in undesirable data truncation. By
2609-
* doing it this way we can be certain that the cast will have default (-1)
2610-
* target typmod.
2611-
*/
2612-
static void
2613-
ri_add_cast_to(StringInfo buf, Oid typid)
2614-
{
2615-
HeapTuple typetup;
2616-
Form_pg_type typform;
2617-
char *typname;
2618-
char *nspname;
2619-
2620-
typetup = SearchSysCache1(TYPEOID, ObjectIdGetDatum(typid));
2621-
if (!HeapTupleIsValid(typetup))
2622-
elog(ERROR, "cache lookup failed for type %u", typid);
2623-
typform = (Form_pg_type) GETSTRUCT(typetup);
2624-
2625-
typname = NameStr(typform->typname);
2626-
nspname = get_namespace_name(typform->typnamespace);
2627-
2628-
appendStringInfo(buf, "::%s.%s",
2629-
quote_identifier(nspname), quote_identifier(typname));
2630-
2631-
ReleaseSysCache(typetup);
2574+
appendStringInfo(buf, " %s ", sep);
2575+
generate_operator_clause(buf, leftop, leftoptype, opoid,
2576+
rightop, rightoptype);
26322577
}
26332578

26342579
/*

0 commit comments

Comments
 (0)