Re: [HACKERS] WAL logging problem in 9.4.3?

At Tue, 18 Feb 2020 23:44:52 -0800, Noah Misch <noah@leadboat.com> wrote in 
> I think attached v35nm is ready for commit to master.  Would anyone like to
> talk me out of back-patching this?  I would not enjoy back-patching it, but
> it's hard to justify lack of back-patch for a data-loss bug.
> 
> Notable changes since v34:
> 
> - Separate a few freestanding fixes into their own patches.

All of the three patches look fine.

> On Mon, Jan 27, 2020 at 07:28:31PM +0900, Kyotaro Horiguchi wrote:
> > --- a/src/backend/catalog/storage.c
> > +++ b/src/backend/catalog/storage.c
> > @@ -388,13 +388,7 @@ RelationPreTruncate(Relation rel)
> >      /* Record largest maybe-unsynced block of files under tracking  */
> >      pending = hash_search(pendingSyncHash, &(rel->rd_smgr->smgr_rnode.node),
> >                            HASH_FIND, NULL);
> > -    if (pending)
> > -    {
> > -        BlockNumber nblocks = smgrnblocks(rel->rd_smgr, MAIN_FORKNUM);
> > -
> > -        if (pending->max_truncated < nblocks)
> > -            pending->max_truncated = nblocks;
> > -    }
> > +    pending->is_truncated = true;
> 
> - Fix this crashing when "pending" is NULL, as it is in this test case:
> 
>   begin;
>   create temp table t ();
>   create table t2 ();  -- cause pendingSyncHash to exist
>   truncate t;
>   rollback;

That's terrible... Thanks for fixint it.

> - Fix the "deleted while still in use" problem that Thomas Munro reported, by
>   removing the heap_create() change.  Restoring the saved rd_createSubid had
>   made obsolete the heap_create() change.  check-world now passes with
>   wal_level=minimal and CLOBBER_CACHE_ALWAYS.

Ok, as in the previous mail.

> - Set rd_droppedSubid in RelationForgetRelation(), not
>   RelationClearRelation().  RelationForgetRelation() knows it is processing a
>   drop, but RelationClearRelation() could only infer that from circumstantial
>   evidence.  This seems more future-proof to me.

Agreed. Different from RelationClearRelatoin, RelationForgetRelation
is called only for "drop"ing the relation.

> - 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.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center