Hi!
I made another version of the patch. This one does UPDATEs for changed
row instead of DELETE/INSERT.
All existing regression tests are still passing (make check).
Mitar
On Mon, Dec 24, 2018 at 4:13 PM Mitar <[email protected]> wrote:
>
> Hi!
>
> Thanks for reply!
>
> On Mon, Dec 24, 2018 at 2:20 PM David Fetter <[email protected]> wrote:
> > You've got the right mailing list, a description of what you want, and
> > a PoC patch. You also got the patch in during the time between
> > Commitfests. You're doing great!
>
> Great!
>
> One thing I am unclear about is how it is determined if this is a
> viable feature to be eventually included. You gave me some suggestions
> to improve in my patch (adding tests and so on). Does this mean that
> the patch should be fully done before a decision is made?
>
> Also, the workflow is that I improve things, and resubmit a patch to
> the mailing list, for now?
>
> > > - Currently only insert and remove operations are done on the
> > > materialized view. This is because the current logic just removes
> > > changed rows and inserts new rows.
> >
> > What other operations might you want to support?
>
> Update. So if a row is changing, instead of doing a remove and insert,
> what currently is being done, I would prefer an update. Then UPDATE
> trigger operation would happen as well. Maybe the INSERT query could
> be changed to INSERT ... ON CONFLICT UPDATE query (not sure if this
> one does UPDATE trigger operation on conflict), and REMOVE changed to
> remove just rows which were really removed, but not only updated.
>
> > As far as you can tell, is this just an efficiency optimization, or
> > might it go to correctness of the behavior?
>
> It is just an optimization. Or maybe even just a surprise. Maybe a
> documentation addition could help here. In my use case I would loop
> over OLD and NEW REFERENCING TABLE so if they are empty, nothing would
> be done. But it is just surprising that DELETE trigger is called even
> when no rows are being deleted in the materialized view.
>
> > I'm not sure I understand the problem being described here. Do you see
> > these as useful to separate for some reason?
>
> So rows which are just updated currently get first DELETE trigger
> called and then INSERT. The issue is that if I am observing this
> behavior from outside, it makes it unclear when I see DELETE if this
> means really that a row has been deleted or it just means that later
> on an INSERT would happen. Now I have to wait for an eventual INSERT
> to determine that. But how long should I wait? It makes consuming
> these notifications tricky.
>
> If I just blindly respond to those notifications, this could introduce
> other problems. For example, if I have a reactive web application it
> could mean a visible flicker to the user. Instead of updating rendered
> row, I would first delete it and then later on re-insert it.
>
> > > Non-concurrent refresh does not trigger any trigger. But it seems
> > > all data to do so is there (previous table, new table), at least for
> > > the statement-level trigger. Row-level triggers could also be
> > > simulated probably (with TRUNCATE and INSERT triggers).
> >
> > Would it make more sense to fill in the missing implementations of NEW
> > and OLD for per-row triggers instead of adding another hack?
>
> You lost me here. But I agree, we should implement this fully, without
> hacks. I just do not know how exactly.
>
> Are you saying that we should support only row-level triggers, or that
> we should support both statement-level and row-level triggers, but
> just make sure we implement this properly? I think that my suggestion
> of using TRUNCATE and INSERT triggers is reasonable in the case of
> full refresh. This is what happens. If we would want to have
> DELETE/UPDATE/INSERT triggers, we would have to compute the diff like
> concurrent version has to do, which would defeat the difference
> between the two. But yes, all INSERT trigger calls should have NEW
> provided.
>
> So per-statement trigger would have TRUNCATE and INSERT called. And
> per-row trigger would have TRUNCATE and per-row INSERTs called.
>
>
> Mitar
>
> --
> http://mitar.tnode.com/
> https://twitter.com/mitar_m
--
http://mitar.tnode.com/
https://twitter.com/mitar_m
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index a171ebabf8..8712a7f61b 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -565,7 +565,7 @@ make_temptable_name_n(char *tempname, int n)
* the old record (if matched) and the ROW from the new table as a single
* column of complex record type (if matched).
*
- * Once we have the diff table, we perform set-based DELETE and INSERT
+ * Once we have the diff table, we perform set-based DELETE, UPDATE, and INSERT
* operations against the materialized view, and discard both temporary
* tables.
*
@@ -590,6 +590,7 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
bool foundUniqueIndex;
List *indexoidlist;
ListCell *indexoidscan;
+ AttrNumber relattno;
int16 relnatts;
Oid *opUsedForQual;
@@ -779,8 +780,9 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
Assert(foundUniqueIndex);
appendStringInfoString(&querybuf,
- " AND newdata OPERATOR(pg_catalog.*=) mv) "
+ ") "
"WHERE newdata IS NULL OR mv IS NULL "
+ "OR newdata OPERATOR(pg_catalog.*<>) mv "
"ORDER BY tid");
/* Create the temporary "diff" table. */
@@ -803,7 +805,7 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
OpenMatViewIncrementalMaintenance();
- /* Deletes must come before inserts; do them first. */
+ /* We do deletes first. */
resetStringInfo(&querybuf);
appendStringInfo(&querybuf,
"DELETE FROM %s mv WHERE ctid OPERATOR(pg_catalog.=) ANY "
@@ -814,7 +816,38 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
if (SPI_exec(querybuf.data, 0) != SPI_OK_DELETE)
elog(ERROR, "SPI_exec failed: %s", querybuf.data);
- /* Inserts go last. */
+ /* Then we do updates. */
+ resetStringInfo(&querybuf);
+ appendStringInfo(&querybuf, "UPDATE %s mv SET (", matviewname);
+
+ for (relattno = 1; relattno <= relnatts; relattno++)
+ {
+ Form_pg_attribute attribute = TupleDescAttr(tupdesc, relattno - 1);
+ char *attributeName = NameStr(attribute->attname);
+
+ /* Ignore dropped */
+ if (attribute->attisdropped)
+ continue;
+
+ if (relattno == 1)
+ {
+ appendStringInfo(&querybuf, "%s", quote_identifier(attributeName));
+ }
+ else
+ {
+ appendStringInfo(&querybuf, ", %s", quote_identifier(attributeName));
+ }
+ }
+
+ appendStringInfo(&querybuf,
+ ") = ROW((diff.newdata).*) FROM %s diff "
+ "WHERE diff.tid IS NOT NULL AND diff.newdata IS NOT NULL "
+ "AND mv.ctid OPERATOR(pg_catalog.=) diff.tid",
+ diffname);
+ if (SPI_exec(querybuf.data, 0) != SPI_OK_UPDATE)
+ elog(ERROR, "SPI_exec failed: %s", querybuf.data);
+
+ /* Inserts and updates go last. */
resetStringInfo(&querybuf);
appendStringInfo(&querybuf,
"INSERT INTO %s SELECT (diff.newdata).* "
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index fb0de60a45..8597def50a 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -208,6 +208,16 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
RelationGetRelationName(rel)),
errdetail("Tables cannot have INSTEAD OF triggers.")));
}
+ else if (rel->rd_rel->relkind == RELKIND_MATVIEW)
+ {
+ /* Materialized views can have only AFTER triggers */
+ if (stmt->timing != TRIGGER_TYPE_AFTER)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("\"%s\" is a materialized view",
+ RelationGetRelationName(rel)),
+ errdetail("Materialized views can have only AFTER triggers.")));
+ }
else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
/* Partitioned tables can't have INSTEAD OF triggers */
@@ -307,7 +317,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
else
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table or view",
+ errmsg("\"%s\" is not a table, view, or materialized view",
RelationGetRelationName(rel))));
if (!allowSystemTableMods && IsSystemRelation(rel))
@@ -1513,11 +1523,12 @@ RemoveTriggerById(Oid trigOid)
if (rel->rd_rel->relkind != RELKIND_RELATION &&
rel->rd_rel->relkind != RELKIND_VIEW &&
+ rel->rd_rel->relkind != RELKIND_MATVIEW &&
rel->rd_rel->relkind != RELKIND_FOREIGN_TABLE &&
rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, view, or foreign table",
+ errmsg("\"%s\" is not a table, view, materialized view, or foreign table",
RelationGetRelationName(rel))));
if (!allowSystemTableMods && IsSystemRelation(rel))
@@ -1619,11 +1630,12 @@ RangeVarCallbackForRenameTrigger(const RangeVar *rv, Oid relid, Oid oldrelid,
/* only tables and views can have triggers */
if (form->relkind != RELKIND_RELATION && form->relkind != RELKIND_VIEW &&
+ form->relkind != RELKIND_MATVIEW &&
form->relkind != RELKIND_FOREIGN_TABLE &&
form->relkind != RELKIND_PARTITIONED_TABLE)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, view, or foreign table",
+ errmsg("\"%s\" is not a table, view, materialized view, or foreign table",
rv->relname)));
/* you must own the table to rename one of its triggers */
