Re: Two issues with REFRESH MATERIALIZED VIEW CONCURRENTLY

Hi Rafia,

Thanks for rebasing both patches.

I re-tested the v7 patches on the current master, all 245 regression tests
passed, and "make check TESTS=matview" runs good.

For issue 1 (v7-0001) the behaviour is correct:
1) A duplicate row containing NULLs is now detected and reported:
REFRESH MATERIALIZED VIEW CONCURRENTLY m;
ERROR: new data for materialized view "m" contains duplicate rows

2) The case where the indexed column itself is NULL still succeeds, since a
unique index treats multiple NULLs as distinct:
CREATE TABLE t(a int, b int);
INSERT INTO t VALUES (NULL, NULL);
CREATE MATERIALIZED VIEW m AS SELECT * FROM t;
CREATE UNIQUE INDEX ON m(a);
INSERT INTO t VALUES (NULL, NULL);
REFRESH MATERIALIZED VIEW CONCURRENTLY m; -- succeeds, m has 2 rows

So I believe v7-0001 is good to go.

While re-testing issue 2 (v7-0002), however, I found that it does not
actually fix the reported performance regression.
Re-running the original example on current master with v7-0002 applied:
CREATE MATERIALIZED VIEW s AS
SELECT generate_series AS x, NULL AS y FROM generate_series(1, 1000000);
CREATE UNIQUE INDEX ON s(x);
REFRESH MATERIALIZED VIEW CONCURRENTLY s; -- 1512 ms
CREATE UNIQUE INDEX ON s(y);
REFRESH MATERIALIZED VIEW CONCURRENTLY s; -- 4650 ms

The second refresh is still ~3x slower, i.e. the regression remains.

The reason is that the optimization skips a nullable column from the join
only when its index has at least one NOT NULL column:

if (index_has_nonnull && !attr->attnotnull)
continue;

and index_has_nonnull is derived from the materialized view column's
attnotnull.
But a materialized view column is never marked NOT NULL:
CREATE MATERIALIZED VIEW does not propagate NOT NULL from the base
relation, and ALTER MATERIALIZED VIEW ... ALTER COLUMN ... SET NOT NULL
is explicitly not supported. So index_has_nonnull is always false, the
column is never skipped, and the patch is effectively a no-op for any
materialized view.

This also explains the discrepancy with my earlier numbers. The first
version of this patch skipped nullable columns unconditionally
(if (!attr->attnotnull) continue;), since all matview columns are nullable,
it skipped every column and reduced the join to
"newdata.* *= mv.*" alone.
That was fast, but it is exactly the behaviour ChangAo showed to be
incorrect for the (NULL, NULL) x 2 case,
because *= treats NULL as equal to NULL.
Adding the index_has_nonnull guard restored correctness but removed the
performance benefit entirely.

In other words, gating on the NOT NULL flag cannot work for materialized
views, the guard the patch does nothing, and without
it the join is incorrect. A working fix for issue 2 would have to rely on
something other than the column's NOT NULL flag, for example
detecting at refresh time whether the indexed column actually contains
NULLs, or a diff strategy that keeps NULLs distinct without forcing a
per-row DELETE+INSERT.

Given the above, I suggest we proceed with the issue 1 patch (v7-0001) on
its own, and treat issue 2 separately, since it needs a different
approach. I will keep exploring options for issue 2 and would welcome any
ideas.

Regards,
Surya Poondla