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.ntt@gmail.com> wrote in 
> At Tue, 18 Feb 2020 23:44:52 -0800, Noah Misch <noah@leadboat.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
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 3068e1e94a..38a2edf860 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -2889,13 +2889,13 @@ include_dir 'conf.d'
       <listitem>
        <para>
         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
-        the new data.  If the data is smaller than this setting, write it to
-        the WAL log; otherwise, use an fsync of the data file.  Depending on
-        the properties of your storage, raising or lowering this value might
-        help if such commits are slowing concurrent transactions.  The default
-        is two megabytes (<literal>2MB</literal>).
+        transaction commits after creating or truncating a permanent table,
+        refreshing a materialized view, or reindexing, this setting determines
+        how to persist the new data.  If the data is smaller than this
+        setting, write it to the WAL log; otherwise, use an fsync of the data
+        file.  Depending on the properties of your storage, raising or
+        lowering this value might help if such commits are slowing concurrent
+        transactions.  The default is two megabytes (<literal>2MB</literal>).
        </para>
       </listitem>
      </varlistentry>
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 391a8a9ea3..682619c9db 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1118,16 +1118,18 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
      */
     {
         Relation    rel1,
-                    rel2;
+                    rel2 PG_USED_FOR_ASSERTS_ONLY;
 
         rel1 = relation_open(r1, NoLock);
+#ifdef USE_ASSERT_CHECKING
         rel2 = relation_open(r2, NoLock);
         rel2->rd_createSubid = rel1->rd_createSubid;
         rel2->rd_newRelfilenodeSubid = rel1->rd_newRelfilenodeSubid;
         rel2->rd_firstRelfilenodeSubid = rel1->rd_firstRelfilenodeSubid;
+        relation_close(rel2, NoLock);
+#endif
         RelationAssumeNewRelfilenode(rel1);
         relation_close(rel1, NoLock);
-        relation_close(rel2, NoLock);
     }
 
     /*
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 7c2181ac2f..3c500944cd 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1985,7 +1985,8 @@ select * from another;
 
 drop table another;
 -- Create an index that skips WAL, then perform a SET DATA TYPE that skips
--- rewriting the index.
+-- rewriting the index. Inadvertent changes on rd_createSubid causes
+-- asseertion failure.
 begin;
 create table skip_wal_skip_rewrite_index (c varchar(10) primary key);
 alter table skip_wal_skip_rewrite_index alter c type varchar(20);
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 6acf31725f..dae7595957 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -332,6 +332,7 @@ ERROR:  set-returning functions are not allowed in DEFAULT expressions
 LINE 1: CREATE TABLE default_expr_agg (a int DEFAULT (generate_serie...
                                                       ^
 -- Verify that subtransaction rollback restores rd_createSubid.
+-- This and the following are expected not causing assertion failure.
 BEGIN;
 CREATE TABLE remember_create_subid (c int);
 SAVEPOINT q; DROP TABLE remember_create_subid; ROLLBACK TO q;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 1b1315f316..ce87ed9ab0 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1361,7 +1361,8 @@ select * from another;
 drop table another;
 
 -- Create an index that skips WAL, then perform a SET DATA TYPE that skips
--- rewriting the index.
+-- rewriting the index. Inadvertent changes on rd_createSubid causes
+-- asseertion failure.
 begin;
 create table skip_wal_skip_rewrite_index (c varchar(10) primary key);
 alter table skip_wal_skip_rewrite_index alter c type varchar(20);
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index a670438c48..3051b5d4e6 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -319,6 +319,7 @@ CREATE TABLE default_expr_agg (a int DEFAULT (select 1));
 CREATE TABLE default_expr_agg (a int DEFAULT (generate_series(1,3)));
 
 -- Verify that subtransaction rollback restores rd_createSubid.
+-- This and the following are expected not causing assertion failure.
 BEGIN;
 CREATE TABLE remember_create_subid (c int);
 SAVEPOINT q; DROP TABLE remember_create_subid; ROLLBACK TO q;