Re: In-placre persistance change of a relation

At Mon, 20 Dec 2021 07:59:29 +0000, Jakub Wartak <Jakub(dot)Wartak(at)tomtom(dot)com> wrote in
> Hi Kyotaro, I'm glad you are still into this
>
> > I didn't register for some reasons.
>
> Right now in v8 there's a typo in ./src/backend/catalog/storage.c :
>
> storage.c: In function 'RelationDropInitFork':
> storage.c:385:44: error: expected statement before ')' token
> pending->unlink_forknum != INIT_FORKNUM)) <-- here, one ) too much

Yeah, I thought that I had removed it. v9 patch I believe is correct.

> > 1. I'm not sure that we want to have the new mark files.
>
> I can't help with such design decision, but if there are doubts maybe then add checking return codes around:
> a) pg_fsync() and fsync_parent_path() (??) inside mdcreatemark()
> b) mdunlinkmark() inside mdunlinkmark()
> and PANIC if something goes wrong?

The point is it is worth the complexity it adds. Since the mark file
can resolve another existing (but I don't recall in detail) issue and
this patchset actually fixes it, it can be said to have a certain
extent of persuasiveness. But that doesn't change the fact that it's
additional complexity.

> > 2. Aside of possible bugs, I'm not confident that the crash-safety of
> > this patch is actually water-tight. At least we need tests for some
> > failure cases.
> >
> > 3. As mentioned in transam/README, failure in removing smgr mark files
> > leads to immediate shut down. I'm not sure this behavior is acceptable.
>
> Doesn't it happen for most of the stuff already? There's even data_sync_retry GUC.

Hmm. Yes, actually it is "as water-tight as possible". I just want
others' eyes on that perspective. CF could be the entry point of
others but I'm a bit hesitent to add a new entry..

> > 4. Including the reasons above, this is not fully functionally.
> > For example, if we execute the following commands on primary,
> > replica dones't work correctly. (boom!)
> >
> > =# CREATE UNLOGGED TABLE t (a int);
> > =# ALTER TABLE t SET LOGGED;
> >
> >
> > The following fixes are done in the attched v8.
> >
> > - Rebased. Referring to Jakub and Justin's work, I replaced direct
> > access to ->rd_smgr with RelationGetSmgr() and removed calls to
> > RelationOpenSmgr(). I still separate the "ALTER TABLE ALL IN
> > TABLESPACE SET LOGGED/UNLOGGED" statement part.
> >
> > - Fixed RelationCreate/DropInitFork's behavior for non-target
> > relations. (From Jakub's work).
> >
> > - Fixed wording of some comments.
> >
> > - As revisited, I found a bug around recovery. If the logged-ness of a
> > relation gets flipped repeatedly in a transaction, duplicate
> > pending-delete entries are accumulated during recovery and work in a
> > wrong way. sgmr_redo now adds up to one entry for a action.
> >
> > - The issue 4 above is not fixed (yet).
>
> Thanks again, If you have any list of crush tests ideas maybe I'll have some minutes
> to try to figure them out. Is there is any goto list of stuff to be checked to add confidence
> to this patch (as per point #2) ?

Just causing a crash (kill -9) after executing problem-prone command
sequence, then seeing recovery works well would sufficient.

For example:

create unlogged table; begin; insert ..; alter table set logged;
<crash>. Recovery works.

"create logged; begin; {alter unlogged; alter logged;} * 1000; alter
logged; commit/abort" doesn't pollute pgdata.

> BTW fast feedback regarding that ALTER patch (there were 4 unlogged tables):
> # ALTER TABLE ALL IN TABLESPACE tbs1 set logged;
> WARNING: unrecognized node type: 349

lol I met a server crash. Will fix. Thanks!

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center