Re: pg_upgrade and materialized views

On Tue, Feb 20, 2018 at 7:05 PM, Claudio Freire <klaussfreire@gmail.com> wrote:
> 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.

Well, the attached script reproduces the issue.

Make a scratch dir somewhere, cd into it, and run it. It will download
9.6.7 and 10.2, build them, create a test db in 9.6.7, modify it a bit
here and there to get to the desired state, pg_upgrade it to 10.2 and
vacuum.

Might even be a good idea to create a regression test with that, but
AFAIK there's nothing remotely like that in the current tests.

The patch you propose makes the test succeed. It may be true that
there might be other similar issues lurking in that code, but it looks
like it fixes this particular issue.