Re: POC: Cleaning up orphaned files using undo logs

On Mon, Jul 1, 2019 at 1:24 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
>
> On Fri, Jun 28, 2019 at 6:09 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > I happened to open up 0001 from this series, which is from Thomas, and
> > I do not think that the pg_buffercache changes are correct. The idea
> > here is that the customer might install version 1.3 or any prior
> > version on an old release, then upgrade to PostgreSQL 13. When they
> > do, they will be running with the old SQL definitions and the new
> > binaries. At that point, it sure looks to me like the code in
> > pg_buffercache_pages.c is going to do the Wrong Thing. [...]
>
> Yep, that was completely wrong. Here's a new version.
>

One comment/question related to
0022-Use-undo-based-rollback-to-clean-up-files-on-abort.patch.

+make_undo_smgr_create(RelFileNode *rnode, FullTransactionId fxid,
+ XLogReaderState *xlog_record)
+{
+ UnpackedUndoRecord undorecord = {0};
+ UndoRecordInsertContext context;
+
+ undorecord.uur_rmid = RM_SMGR_ID;
+ undorecord.uur_type = UNDO_SMGR_CREATE;
+ undorecord.uur_info = UREC_INFO_PAYLOAD;
+ undorecord.uur_dbid = rnode->dbNode;
+ undorecord.uur_xid = XidFromFullTransactionId(fxid);
+ undorecord.uur_cid = InvalidCommandId;
+ undorecord.uur_fork = InvalidForkNumber;

While reviewing Dilip's patch(undo-record-interface), I noticed that
we include Fork_Num in undo record, if it is not a MAIN_FORKNUM. So,
in this patch's case, we will always include it as you are passing
InvalidForkNumber. I also see that the patch doesn't use uur_fork in
the undo record handler, so I think you don't care what is its value.
I am not sure what is the best thing to do here, but it might be
better if we can avoiding adding fork_num in each undo record.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com