8000 Fix some corner-case issues in REFRESH MATERIALIZED VIEW CONCURRENTLY. · postgres/postgres@aa1cacd · GitHub
[go: up one dir, main page]
Skip to content

Commit aa1cacd

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 c553e4a commit aa1cacd

File tree

4 files changed

+203
-111
lines changed

4 files changed

+203
-111
lines changed

‎src/backend/commands/matview.c‎

Lines changed: 112 additions & 49 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

@@ -490,25 +488,6 @@ make_temptable_name_n(char *tempname, int n)
490488
return namebuf.data;
491489
}
492490

493-
static void
494-
mv_GenerateOper(StringInfo buf, Oid opoid)
495-
{
496-
HeapTuple opertup;
497-
Form_pg_operator operform;
498-
499-
opertup = SearchSysCache1(OPEROID, ObjectIdGetDatum(opoid));
500-
if (!HeapTupleIsValid(opertup))
501-
elog(ERROR, "cache lookup failed for operator %u", opoid);
502-
operform = (Form_pg_operator) GETSTRUCT(opertup);
503-
Assert(operform->oprkind == 'b');
504-
505-
appendStringInfo(buf, "OPERATOR(%s.%s)",
506-
quote_identifier(get_namespace_name(operform->oprnamespace)),
507-
NameStr(operform->oprname));
508-
509-
ReleaseSysCache(opertup);
510-
}
511-
512491
/*
51 6D40 3492
* refresh_by_match_merge
514493
*
@@ -556,7 +535,7 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
556535
List *indexoidlist;
557536
ListCell *indexoidscan;
558537
int16 relnatts;
559-
bool *usedForQual;
538+
Oid *opUsedForQual;
560539

561540
initStringInfo(&querybuf);
562541
matviewRel = heap_open(matviewOid, NoLock);
@@ -568,7 +547,6 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
568547
diffname = make_temptable_name_n(tempname, 2);
569548

570549
relnatts = matviewRel->rd_rel->relnatts;
571-
usedForQual = (bool *) palloc0(sizeof(bool) * relnatts);
572550

573551
/* Open SPI context. */
574552
if (SPI_connect() != SPI_OK_CONNECT)
@@ -632,58 +610,98 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
632610
* include all rows.
633611
*/
634612
tupdesc = matviewRel->rd_att;
613+
opUsedForQual = (Oid *) palloc0(sizeof(Oid) * relnatts);
635614
foundUniqueIndex = false;
615+
636616
indexoidlist = RelationGetIndexList(matviewRel);
637617

638618
foreach(indexoidscan, indexoidlist)
639619
{
640620
Oid indexoid = lfirst_oid(indexoidscan);
641621
Relation indexRel;
642-
Form_pg_index indexStruct;
643622

644623
indexRel = index_open(indexoid, RowExclusiveLock);
645-
indexStruct = indexRel->rd_index;
646-
647-
/*
648-
* We're only interested if it is unique, valid, contains no
649-
* expressions, and is not partial.
650-
*/
651-
if (indexStruct->indisunique &&
652-
IndexIsValid(indexStruct) &&
653-
RelationGetIndexExpressions(indexRel) == NIL &&
654-
RelationGetIndexPredicate(indexRel) == NIL)
624+
if (is_usable_unique_index(indexRel))
655625
{
626+
Form_pg_index indexStruct = indexRel->rd_index;
656627
int numatts = indexStruct->indnatts;
628+
oidvector *indclass;
629+
Datum indclassDatum;
630+
bool isnull;
657631
int i;
658632

633+
/* Must get indclass the hard way. */
634+
indclassDatum = SysCacheGetAttr(INDEXRELID,
635+
indexRel->rd_indextuple,
636+
Anum_pg_index_indclass,
637+
&isnull);
638+
Assert(!isnull);
639+
indclass = (oidvector *) DatumGetPointer(indclassDatum);
640+
659641
/* Add quals for all columns from this index. */
660642
for (i = 0; i < numatts; i++)
661643
{
662644
int attnum = indexStruct->indkey.values[i];
663-
Oid type;
645+
Oid opclass = indclass->values[i];
646+
Form_pg_attribute attr = TupleDescAttr(tupdesc, attnum - 1);
647+
Oid attrtype = attr->atttypid;
648+
HeapTuple cla_ht;
649+
Form_pg_opclass cla_tup;
650+
Oid opfamily;
651+
Oid opcintype;
664652
Oid op;
665-
const char *colname;
653+
const char *leftop;
654+
const char *rightop;
655+
656+
/*
657+
* Identify the equality operator associated with this index
658+
* column. First we need to look up the column's opclass.
659+
*/
660+
cla_ht = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclass));
661+
if (!HeapTupleIsValid(cla_ht))
662+
elog(ERROR, "cache lookup failed for opclass %u", opclass);
663+
cla_tup = (Form_pg_opclass) GETSTRUCT(cla_ht);
664+
Assert(cla_tup->opcmethod == BTREE_AM_OID);
665+
opfamily = cla_tup->opcfamily;
666+
opcintype = cla_tup->opcintype;
667+
ReleaseSysCache(cla_ht);
668+
669+
op = get_opfamily_member(opfamily, opcintype, opcintype,
670+
BTEqualStrategyNumber);
671+
if (!OidIsValid(op))
672+
elog(ERROR, "missing operator %d(%u,%u) in opfamily %u",
673+
BTEqualStrategyNumber, opcintype, opcintype, opfamily);
666674

667675
/*
668-
* Only include the column once regardless of how many times
669-
* it shows up in how many indexes.
676+
* If we find the same column with the same equality semantics
677+
* in more than one index, we only need to emit the equality
678+
* clause once.
679+
*
680+
* Since we only remember the last equality operator, this
681+
* code could be fooled into emitting duplicate clauses given
682+
* multiple indexes with several different opclasses ... but
683+
* that's so unlikely it doesn't seem worth spending extra
684+
* code to avoid.
670685
*/
671-
if (usedForQual[attnum - 1])
686+
if (opUsedForQual[at 9E88 tnum - 1] == op)
672687
continue;
673-
usedForQual[attnum - 1] = true;
688+
opUsedForQual[attnum - 1] = op;
674689

675690
/*
676691
* Actually add the qual, ANDed with any others.
677692
*/
678693
if (foundUniqueIndex)
679694
appendStringInfoString(&querybuf, " AND ");
680695

681-
colname = quote_identifier(NameStr((tupdesc->attrs[attnum - 1])->attname));
682-
appendStringInfo(&querybuf, "newdata.%s ", colname);
683-
type = attnumTypeId(matviewRel, attnum);
684-
op = lookup_type_cache(type, TYPECACHE_EQ_OPR)->eq_opr;
685-
mv_GenerateOper(&querybuf, op);
686-
appendStringInfo(&querybuf, " mv.%s", colname);
696+
leftop = quote_qualified_identifier("newdata",
697+
NameStr(attr->attname));
698+
rightop = quote_qualified_identifier("mv",
699+
NameStr(attr->attname));
700+
701+
generate_operator_clause(&querybuf,
702+
leftop, attrtype,
703+
op,
704+
rightop, attrtype);
687705

688706
foundUniqueIndex = true;
689707
}
@@ -775,6 +793,51 @@ refresh_by_heap_swap(Oid matviewOid, Oid OIDNewHeap, char relpersistence)
775793
RecentXmin, ReadNextMultiXactId(), relpersistence);
776794
}
777795

796+
/*
797+
* Check whether specified index is usable for match merge.
798+
*/
799+
static bool
800+
is_usable_unique_index(Relation indexRel)
801+
{
802+
Form_pg_index indexStruct = indexRel->rd_index;
803+
804+
/*
805+
* Must be unique, valid, immediate, non-partial, and be defined over
806+
* plain user columns (not expressions). We also require it to be a
807+
* btree. Even if we had any other unique index kinds, we'd not know how
808+
* to identify the corresponding equality operator, nor could we be sure
809+
* that the planner could implement the required FULL JOIN with non-btree
810+
* operators.
811+
*/
812+
if (indexStruct->indisunique &&
813+
indexStruct->indimmediate &&
814+
indexRel->rd_rel->relam == BTREE_AM_OID &&
815+
IndexIsValid(indexStruct) &&
816+
RelationGetIndexPredicate(indexRel) == NIL &&
817+
indexStruct->indnatts > 0)
818+
{
819+
/*
820+
* The point of groveling through the index columns individually is to
821+
* reject both index expressions and system columns. Currently,
822+
* matviews couldn't have OID columns so there's no way to create an
823+
* index on a system column; but maybe someday that wouldn't be true,
824+
* so let's be safe.
825+
*/
826+
int numatts = indexStruct->indnatts;
827+
int i;
828+
829+
for (i = 0; i < numatts; i++)
830+
{
831+
int attnum = indexStruct->indkey.values[i];
832+
833+
if (attnum <= 0)
834+
return false;
835+
}
836+
return true;
837+
}
 D306
838+
return false;
839+
}
840+
778841

779842
/*
780843
* 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)
0