Skip to content

Navigation Menu

Sign in

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit e1f186d

Browse filesBrowse 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 b6ba94e commit e1f186d
Copy full SHA for e1f186d

File tree

4 files changed

+203
-111
lines changed
Filter options

4 files changed

+203
-111
lines changed

‎src/backend/commands/matview.c

Copy file name to clipboardExpand all lines: src/backend/commands/matview.c
+112-49Lines changed: 112 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@
2020
#include "catalog/catalog.h"
2121
#include "catalog/indexing.h"
2222
#include "catalog/namespace.h"
23+
#include "catalog/pg_am.h"
24+
#include "catalog/pg_opclass.h"
2325
#include "catalog/pg_operator.h"
2426
#include "commands/cluster.h"
2527
#include "commands/matview.h"
@@ -38,7 +40,6 @@
3840
#include "utils/rel.h"
3941
#include "utils/snapmgr.h"
4042
#include "utils/syscache.h"
41-
#include "utils/typcache.h"
4243

4344

4445
typedef struct
@@ -60,14 +61,11 @@ static void transientrel_shutdown(DestReceiver *self);
6061
static void transientrel_destroy(DestReceiver *self);
6162
static void refresh_matview_datafill(DestReceiver *dest, Query *query,
6263
const char *queryString);
63-
6464
static char *make_temptable_name_n(char *tempname, int n);
65-
static void mv_GenerateOper(StringInfo buf, Oid opoid);
66-
6765
static void refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
6866
int save_sec_context);
6967
static void refresh_by_heap_swap(Oid matviewOid, Oid OIDNewHeap);
70-
68+
static bool is_usable_unique_index(Relation indexRel);
7169
static void OpenMatViewIncrementalMaintenance(void);
7270
static void CloseMatViewIncrementalMaintenance(void);
7371

@@ -477,25 +475,6 @@ make_temptable_name_n(char *tempname, int n)
477475
return namebuf.data;
478476
}
479477

480-
static void
481-
mv_GenerateOper(StringInfo buf, Oid opoid)
482-
{
483-
HeapTuple opertup;
484-
Form_pg_operator operform;
485-
486-
opertup = SearchSysCache1(OPEROID, ObjectIdGetDatum(opoid));
487-
if (!HeapTupleIsValid(opertup))
488-
elog(ERROR, "cache lookup failed for operator %u", opoid);
489-
operform = (Form_pg_operator) GETSTRUCT(opertup);
490-
Assert(operform->oprkind == 'b');
491-
492-
appendStringInfo(buf, "OPERATOR(%s.%s)",
493-
quote_identifier(get_namespace_name(operform->oprnamespace)),
494-
NameStr(operform->oprname));
495-
496-
ReleaseSysCache(opertup);
497-
}
498-
499478
/*
500479
* refresh_by_match_merge
501480
*
@@ -543,7 +522,7 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
543522
List *indexoidlist;
544523
ListCell *indexoidscan;
545524
int16 relnatts;
546-
bool *usedForQual;
525+
Oid *opUsedForQual;
547526

548527
initStringInfo(&querybuf);
549528
matviewRel = heap_open(matviewOid, NoLock);
@@ -555,7 +534,6 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
555534
diffname = make_temptable_name_n(tempname, 2);
556535

557536
relnatts = matviewRel->rd_rel->relnatts;
558-
usedForQual = (bool *) palloc0(sizeof(bool) * relnatts);
559537

560538
/* Open SPI context. */
561539
if (SPI_connect() != SPI_OK_CONNECT)
@@ -619,58 +597,98 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
619597
* include all rows.
620598
*/
621599
tupdesc = matviewRel->rd_att;
600+
opUsedForQual = (Oid *) palloc0(sizeof(Oid) * relnatts);
622601
foundUniqueIndex = false;
602+
623603
indexoidlist = RelationGetIndexList(matviewRel);
624604

625605
foreach(indexoidscan, indexoidlist)
626606
{
627607
Oid indexoid = lfirst_oid(indexoidscan);
628608
Relation indexRel;
629-
Form_pg_index indexStruct;
630609

631610
indexRel = index_open(indexoid, RowExclusiveLock);
632-
indexStruct = indexRel->rd_index;
633-
634-
/*
635-
* We're only interested if it is unique, valid, contains no
636-
* expressions, and is not partial.
637-
*/
638-
if (indexStruct->indisunique &&
639-
IndexIsValid(indexStruct) &&
640-
RelationGetIndexExpressions(indexRel) == NIL &&
641-
RelationGetIndexPredicate(indexRel) == NIL)
611+
if (is_usable_unique_index(indexRel))
642612
{
613+
Form_pg_index indexStruct = indexRel->rd_index;
643614
int numatts = indexStruct->indnatts;
615+
oidvector *indclass;
616+
Datum indclassDatum;
617+
bool isnull;
644618
int i;
645619

620+
/* Must get indclass the hard way. */
621+
indclassDatum = SysCacheGetAttr(INDEXRELID,
622+
indexRel->rd_indextuple,
623+
Anum_pg_index_indclass,
624+
&isnull);
625+
Assert(!isnull);
626+
indclass = (oidvector *) DatumGetPointer(indclassDatum);
627+
646628
/* Add quals for all columns from this index. */
647629
for (i = 0; i < numatts; i++)
648630
{
649631
int attnum = indexStruct->indkey.values[i];
650-
Oid type;
632+
Oid opclass = indclass->values[i];
633+
Form_pg_attribute attr = TupleDescAttr(tupdesc, attnum - 1);
634+
Oid attrtype = attr->atttypid;
635+
HeapTuple cla_ht;
636+
Form_pg_opclass cla_tup;
637+
Oid opfamily;
638+
Oid opcintype;
651639
Oid op;
652-
const char *colname;
640+
const char *leftop;
641+
const char *rightop;
642+
643+
/*
644+
* Identify the equality operator associated with this index
645+
* column. First we need to look up the column's opclass.
646+
*/
647+
cla_ht = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclass));
648+
if (!HeapTupleIsValid(cla_ht))
649+
elog(ERROR, "cache lookup failed for opclass %u", opclass);
650+
cla_tup = (Form_pg_opclass) GETSTRUCT(cla_ht);
651+
Assert(cla_tup->opcmethod == BTREE_AM_OID);
652+
opfamily = cla_tup->opcfamily;
653+
opcintype = cla_tup->opcintype;
654+
ReleaseSysCache(cla_ht);
655+
656+
op = get_opfamily_member(opfamily, opcintype, opcintype,
657+
BTEqualStrategyNumber);
658+
if (!OidIsValid(op))
659+
elog(ERROR, "missing operator %d(%u,%u) in opfamily %u",
660+
BTEqualStrategyNumber, opcintype, opcintype, opfamily);
653661

654662
/*
655-
* Only include the column once regardless of how many times
656-
* it shows up in how many indexes.
663+
* If we find the same column with the same equality semantics
664+
* in more than one index, we only need to emit the equality
665+
* clause once.
666+
*
667+
* Since we only remember the last equality operator, this
668+
* code could be fooled into emitting duplicate clauses given
669+
* multiple indexes with several different opclasses ... but
670+
* that's so unlikely it doesn't seem worth spending extra
671+
* code to avoid.
657672
*/
658-
if (usedForQual[attnum - 1])
673+
if (opUsedForQual[attnum - 1] == op)
659674
continue;
660-
usedForQual[attnum - 1] = true;
675+
opUsedForQual[attnum - 1] = op;
661676

662677
/*
663678
* Actually add the qual, ANDed with any others.
664679
*/
665680
if (foundUniqueIndex)
666681
appendStringInfoString(&querybuf, " AND ");
667682

668-
colname = quote_identifier(NameStr((tupdesc->attrs[attnum - 1])->attname));
669-
appendStringInfo(&querybuf, "newdata.%s ", colname);
670-
type = attnumTypeId(matviewRel, attnum);
671-
op = lookup_type_cache(type, TYPECACHE_EQ_OPR)->eq_opr;
672-
mv_GenerateOper(&querybuf, op);
673-
appendStringInfo(&querybuf, " mv.%s", colname);
683+
leftop = quote_qualified_identifier("newdata",
684+
NameStr(attr->attname));
685+
rightop = quote_qualified_identifier("mv",
686+
NameStr(attr->attname));
687+
688+
generate_operator_clause(&querybuf,
689+
leftop, attrtype,
690+
op,
691+
rightop, attrtype);
674692

675693
foundUniqueIndex = true;
676694
}
@@ -762,6 +780,51 @@ refresh_by_heap_swap(Oid matviewOid, Oid OIDNewHeap)
762780
RecentXmin, ReadNextMultiXactId());
763781
}
764782

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

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

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

Copy file name to clipboardExpand all lines: src/backend/utils/adt/ri_triggers.c
+7-62Lines changed: 7 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,6 @@ static void ri_GenerateQual(StringInfo buf,
207207
const char *leftop, Oid leftoptype,
208208
Oid opoid,
209209
const char *rightop, Oid rightoptype);
210-
static void ri_add_cast_to(StringInfo buf, Oid typid);
211210
static void ri_GenerateQualCollation(StringInfo buf, Oid collation);
212211
static int ri_NullCheck(HeapTuple tup,
213212
const RI_ConstraintInfo *riinfo, bool rel_is_pk);
@@ -2547,13 +2546,10 @@ quoteRelationName(char *buffer, Relation rel)
25472546
/*
25482547
* ri_GenerateQual --- generate a WHERE clause equating two variables
25492548
*
2550-
* The idea is to append " sep leftop op rightop" to buf. The complexity
2551-
* comes from needing to be sure that the parser will select the desired
2552-
* operator. We always name the operator using OPERATOR(schema.op) syntax
2553-
* (readability isn't a big priority here), so as to avoid search-path
2554-
* uncertainties. We have to emit casts too, if either input isn't already
2555-
* the input type of the operator; else we are at the mercy of the parser's
2556-
* heuristics for ambiguous-operator resolution.
2549+
* This basically appends " sep leftop op rightop" to buf, adding casts
2550+
* and schema qualification as needed to ensure that the parser will select
2551+
* the operator we specify. leftop and rightop should be parenthesized
2552+
* if they aren't variables or parameters.
25572553
*/
25582554
static void
25592555
ri_GenerateQual(StringInfo buf,
@@ -2562,60 +2558,9 @@ ri_GenerateQual(StringInfo buf,
25622558
Oid opoid,
25632559
const char *rightop, Oid rightoptype)
25642560
{
2565-
HeapTuple opertup;
2566-
Form_pg_operator operform;
2567-
char *oprname;
2568-
char *nspname;
2569-
2570-
opertup = SearchSysCache1(OPEROID, ObjectIdGetDatum(opoid));
2571-
if (!HeapTupleIsValid(opertup))
2572-
elog(ERROR, "cache lookup failed for operator %u", opoid);
2573-
operform = (Form_pg_operator) GETSTRUCT(opertup);
2574-
Assert(operform->oprkind == 'b');
2575-
oprname = NameStr(operform->oprname);
2576-
2577-
nspname = get_namespace_name(operform->oprnamespace);
2578-
2579-
appendStringInfo(buf, " %s %s", sep, leftop);
2580-
if (leftoptype != operform->oprleft)
2581-
ri_add_cast_to(buf, operform->oprleft);
2582-
appendStringInfo(buf, " OPERATOR(%s.", quote_identifier(nspname));
2583-
appendStringInfoString(buf, oprname);
2584-
appendStringInfo(buf, ") %s", rightop);
2585-
if (rightoptype != operform->oprright)
2586-
ri_add_cast_to(buf, operform->oprright);
2587-
2588-
ReleaseSysCache(opertup);
2589-
}
2590-
2591-
/*
2592-
* Add a cast specification to buf. We spell out the type name the hard way,
2593-
* intentionally not using format_type_be(). This is to avoid corner cases
2594-
* for CHARACTER, BIT, and perhaps other types, where specifying the type
2595-
* using SQL-standard syntax results in undesirable data truncation. By
2596-
* doing it this way we can be certain that the cast will have default (-1)
2597-
* target typmod.
2598-
*/
2599-
static void
2600-
ri_add_cast_to(StringInfo buf, Oid typid)
2601-
{
2602-
HeapTuple typetup;
2603-
Form_pg_type typform;
2604-
char *typname;
2605-
char *nspname;
2606-
2607-
typetup = SearchSysCache1(TYPEOID, ObjectIdGetDatum(typid));
2608-
if (!HeapTupleIsValid(typetup))
2609-
elog(ERROR, "cache lookup failed for type %u", typid);
2610-
typform = (Form_pg_type) GETSTRUCT(typetup);
2611-
2612-
typname = NameStr(typform->typname);
2613-
nspname = get_namespace_name(typform->typnamespace);
2614-
2615-
appendStringInfo(buf, "::%s.%s",
2616-
quote_identifier(nspname), quote_identifier(typname));
2617-
2618-
ReleaseSysCache(typetup);
2561+
appendStringInfo(buf, " %s ", sep);
2562+
generate_operator_clause(buf, leftop, leftoptype, opoid,
2563+
rightop, rightoptype);
26192564
}
26202565

26212566
/*

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.