RE: In-placre persistance change of a relation

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

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

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

> 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) ?

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

-J.