Sorry, just one fix. (omitting some typos, though..)
At Wed, 19 Feb 2020 17:29:08 +0900 (JST), Kyotaro Horiguchi
<horikyota....@gmail.com> wrote in
> At Tue, 18 Feb 2020 23:44:52 -0800, Noah Misch <n...@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.
Hmm. I'm not sure that index_create having the new subid parameters is
good. And the last if(OidIsValid) clause in AtExecAddIndex 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
