Re: [HACKERS] WAL logging problem in 9.4.3?

Hello. I looked through the latest patch.

At Wed, 19 Feb 2020 17:29:08 +0900 (JST), Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> wrote in
> At Tue, 18 Feb 2020 23:44:52 -0800, Noah Misch <noah(at)leadboat(dot)com> wrote in
> > - When reusing an index build, instead of storing the dropped relid in the
> > IndexStmt and opening the dropped relcache entry in ATExecAddIndex(), store
> > the subid fields in the IndexStmt. This is less code, and I felt
> > RelationIdGetRelationCache() invited misuse.
>
> Hmm. I'm not sure that index_create having the new subid parameters is
> good. And the last if(OidIsValid) clause handles storage persistence
> so I did that there. But I don't strongly against it.
>
> Please give a bit more time to look it.

The change on alter_table.sql and create_table.sql is expecting to
cause assertion failure. Don't we need that kind of explanation in
the comment?

In swap_relation_files, we can remove rel2-related code when #ifndef
USE_ASSERT_CHECKING.

The patch adds the test for createSubid to pg_visibility.out. It
doesn't fail without CLOBBER_CACHE_ALWAYS while regression test but
CLOBBER_CACHE_ALWAYS causes initdb fail and the added check won't be
reached. I'm not sure it is useful.

config.sgml:
+ When <varname>wal_level</varname> is <literal>minimal</literal> and a
+ transaction commits after creating or rewriting a permanent table,
+ materialized view, or index, this setting determines how to persist

"creating or truncation" a permanent table? and maybe "refreshing
matview and reindex". I'm not sure that they can be merged that way.

Other than the item related to pg_visibility.sql are in the attached.

The others look good to me.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center