Re: POC: Cleaning up orphaned files using undo logs

Hi Thomas,

Few review comments on 0003-Add-undo-log-manager.patch:
1) Upgrade may fail
+/*
+ * Compute the new redo, and move the pg_undo file to match if necessary.
+ * Rather than renaming it, we'll create a new copy, so that a failure that
+ * occurs before the controlfile is rewritten won't be fatal.
+ */
+static void
+AdjustRedoLocation(const char *DataDir)
+{
+ uint64 old_redo = ControlFile.checkPointCopy.redo;
+ char old_pg_undo_path[MAXPGPATH];
+ char new_pg_undo_path[MAXPGPATH];
+ int old_fd;
+ int new_fd;
+ ssize_t nread;
+ ssize_t nwritten;
+ char buffer[1024];
+
+ /*
+ * Adjust fields as needed to force an empty XLOG starting at
+ * newXlogSegNo.
+ */

During the upgrade we delete the undo files present in the new cluster
and copy the undo files from the old cluster to the new cluster.
Then we try to readjust the redo location using pg_resetwal.
While trying to readjust we get the current control file details
from current cluster. We try to open the current undo file
present in the cluster using the details from the current cluster.
As the undo files from the current cluster have been removed
and replaced with the old cluster contents, the file open will fail.

Attached a patch to solve this problem.

2) drop table space failure in corner case.

+ else
+ {
+ /*
+ * There is data we need in this undo log. We can't force it to
+ * be detached.
+ */
+ ok = false;
+ }
+ LWLockRelease(&slot->mutex);

+ /* If we failed, then give up now and report failure. */
+ if (!ok)
+ return false;

One thought, can we discard the current tablespace entries
and try not to fail.

3) There will be a problem if some files deletion is successful and some
file deletion fails, the meta contents having end details also need to be
applied or to handle the case where the undo is created further after
rollback

+ while ((de = ReadDirExtended(dir, undo_path, LOG)) != NULL)
+ {
+ char segment_path[MAXPGPATH];
+
+ if (strcmp(de->d_name, ".") == 0 ||
+ strcmp(de->d_name, "..") == 0)
+ continue;
+ snprintf(segment_path, sizeof(segment_path), "%s/%s",
+ undo_path, de->d_name);
+ if (unlink(segment_path) < 0)
+ elog(LOG, "couldn't unlink file \"%s\": %m", segment_path);
+ }

4) In error case unlinked undo segment message will be logged
+ while ((de = ReadDirExtended(dir, undo_path, LOG)) != NULL)
+ {
+ char segment_path[MAXPGPATH];
+
+ if (strncmp(de->d_name, segment_prefix, segment_prefix_size) != 0)
+ continue;
+ snprintf(segment_path, sizeof(segment_path), "%s/%s",
+ undo_path, de->d_name);
+ elog(DEBUG1, "unlinked undo segment \"%s\"", segment_path);
+ if (unlink(segment_path) < 0)
+ elog(LOG, "couldn't unlink file \"%s\": %m", segment_path);
+ }
+ FreeDir(dir);

In error case the success message will be logged.

5) UndoRecPtrIsValid can be used to check InvalidUndoRecPtr
+ /*
+ * 'size' is expressed in usable non-header bytes. Figure out how far we
+ * have to move insert to create space for 'size' usable bytes, stepping
+ * over any intervening headers.
+ */
+ Assert(slot->meta.unlogged.insert % BLCKSZ >= UndoLogBlockHeaderSize);
+ if (context->try_location != InvalidUndoRecPtr)

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com
On Thu, Jul 25, 2019 at 9:30 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Thu, Jul 25, 2019 at 7:48 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Wed, Jul 24, 2019 at 11:04 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > >
> > > Hi,
> > >
> > > I have done some review of undolog patch series
> > > and here are my comments:
> > > 0003-Add-undo-log-manager.patch
> > >
> > > 1) As undo log is being created in tablespace,
> > > if the tablespace is dropped later, will it have any impact?
>
> Thanks Amit, that clarifies the problem I was thinking.
> I have another question regarding drop table space failure, but I
> don't have a better solution for that problem.
> Let me think more about it and discuss.
> >
> > Yes, it drops the undo logs present in tablespace being dropped. See
> > DropUndoLogsInTablespace() in the same patch.
> >
> > >
> > > 4) Should we add a readme file for undolog as it does a fair amount of work
> > > and is core part of the undo system?
> > >
> Thanks Amit, I could get the details of readme.
> >
> > The Readme is already present in the patch series posted by Thomas.
> > See 0019-Add-developer-documentation-for-the-undo-log-storage.patch in
> > email [1].
> >
> > [1] - https://www.postgresql.org/message-id/CA%2BhUKGKni7EEU4FT71vZCCwPeaGb2PQOeKOFjQJavKnD577UMQ%40mail.gmail.com
> >
> > --
> > With Regards,
> > Amit Kapila.
> > EnterpriseDB: http://www.enterprisedb.com
>
> --
> Regards,
> Vignesh
> EnterpriseDB: http://www.enterprisedb.com