Re: pg_upgrade and materialized views

On Tue, Feb 20, 2018 at 6:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Claudio Freire <klaussfreire@gmail.com> writes:
>> On Tue, Feb 20, 2018 at 6:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> (2) independently of that, it sounds like REFRESH
>>> MATERIALIZED VIEW CONCURRENTLY is somehow preventing advancement of the
>>> matview's relfrozenxid in the source DB.
>
>> Not necessarily. I have vacuum_table_freeze_max_age set to 350M, so
>> it's not yet due for freezing.
>
> Perhaps, but it seems pretty suggestive that all of the non-concurrently
> refreshed matviews have relfrozenxid significantly newer than all of
> the concurrently refreshed ones.  Maybe that's just coincidence, or a
> predictable outcome of your usage pattern, but I think it needs
> explaining.

I think it's quite expectable.

The ones that use concurrently are expected to change only slightly
between refreshes. All those matviews get refreshed periodically in
mostly the same schedule, but some are either slow to refresh
concurrently, and thus we prefer a full refresh, or are expected to
change too much for a concurrent refresh to be useful. As such,
concurrently refreshed views are expected to have rows that remain
valid for a long while (old data that has stabilized, the views
themselves represent a few month's worth of data). Fully refreshed
views will always have recent xids because they are recreated often.
I'd say it makes sense.


On Tue, Feb 20, 2018 at 6:54 PM, Andres Freund <andres@anarazel.de> wrote:
> The important part then happens in pg_dump. Note
>
>                 /*
>                  * To create binary-compatible heap files, we have to ensure the same
>                  * physical column order, including dropped columns, as in the
>                  * original.  Therefore, we create dropped columns above and drop them
>                  * here, also updating their attlen/attalign values so that the
>                  * dropped column can be skipped properly.  (We do not bother with
>                  * restoring the original attbyval setting.)  Also, inheritance
>                  * relationships are set up by doing ALTER TABLE INHERIT rather than
>                  * using an INHERITS clause --- the latter would possibly mess up the
>                  * column order.  That also means we have to take care about setting
>                  * attislocal correctly, plus fix up any inherited CHECK constraints.
>                  * Analogously, we set up typed tables using ALTER TABLE / OF here.
>                  */
>                 if (dopt->binary_upgrade &&
>                         (tbinfo->relkind == RELKIND_RELATION ||
>                          tbinfo->relkind == RELKIND_FOREIGN_TABLE ||
>                          tbinfo->relkind == RELKIND_PARTITIONED_TABLE))
>                 {
> ...
>                         appendPQExpBufferStr(q, "\n-- For binary upgrade, set heap's relfrozenxid and
relminmxid\n");
>                         appendPQExpBuffer(q, "UPDATE pg_catalog.pg_class\n"
>                                                           "SET relfrozenxid = '%u', relminmxid = '%u'\n"
>                                                           "WHERE oid = ",
>                                                           tbinfo->frozenxid, tbinfo->minmxid);
>                         appendStringLiteralAH(q, fmtId(tbinfo->dobj.name), fout);
>                         appendPQExpBufferStr(q, "::pg_catalog.regclass;\n");
>
> note that the above if clause doesn't include materialized tables. Which
> sems to explain this bug?  Could you check that just updating the above
> if to include matviews fixes the bug for you?

The pg_dump --binary-only test does produce the necessary SQL to set
relfrozenxid after that change, so it looks like it would fix it.

To be able to fully confirm it, though, I'll have to build a mimal
case to reproduce the issue, because the snapshot I used it not usable
again (can't re-upgrade), and launching another snapshot takes a lot
of time. Basically, I'll get back to you with a confirmation, but it
does look good.

> Looking into this I also saw:
>
> /*
>  *      set_frozenxids()
>  *
>  *      We have frozen all xids, so set datfrozenxid, relfrozenxid, and
>  *      relminmxid to be the old cluster's xid counter, which we just set
>  *      in the new cluster.  User-table frozenxid and minmxid values will
>  *      be set by pg_dump --binary-upgrade, but objects not set by the pg_dump
>  *      must have proper frozen counters.
>  */
> static
> void
> set_frozenxids(bool minmxid_only)
> ...
>                         /* set pg_class.relfrozenxid */
>                         PQclear(executeQueryOrDie(conn,
>                                                                           "UPDATE       pg_catalog.pg_class "
>                                                                           "SET  relfrozenxid = '%u' "
>                         /* only heap, materialized view, and TOAST are vacuumed */
>                                                                           "WHERE        relkind IN ("
>                                                                           CppAsString2(RELKIND_RELATION) ", "
>                                                                           CppAsString2(RELKIND_MATVIEW) ", "
>                                                                           CppAsString2(RELKIND_TOASTVALUE) ")",
>                                                                           old_cluster.controldata.chkpnt_nxtxid));
>
> which makes a bit uncomfortable, but I can't quite put my finger on
> why.

I looked into that one, it's not relevant to this case, since it's
working on template1 (check the conn used there).