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? +void +UndoLogDirectory(Oid tablespace, char *dir) +{ + if (tablespace == DEFAULTTABLESPACE_OID || + tablespace == InvalidOid) + snprintf(dir, MAXPGPATH, "base/undo"); + else + snprintf(dir, MAXPGPATH, "pg_tblspc/%u/%s/undo", + tablespace, TABLESPACE_VERSION_DIRECTORY); +} 2) Header file exclusion a) The following headers can be excluded in undolog.c +#include "access/transam.h" +#include "access/undolog.h" +#include "access/xlogreader.h" +#include "catalog/catalog.h" +#include "nodes/execnodes.h" +#include "storage/buf.h" +#include "storage/bufmgr.h" +#include "storage/fd.h" +#include "storage/lwlock.h" +#include "storage/shmem.h" +#include "storage/standby.h" +#include "storage/sync.h" +#include "utils/memutils.h" b) The following headers can be excluded from undofile.c +#include "access/undolog.h" +#include "catalog/database_internal.h" +#include "miscadmin.h" +#include "postmaster/bgwriter.h" +#include "storage/fd.h" +#include "storage/smgr.h" +#include "utils/memutils.h" 3) Some macro replacement. a)Session.h +++ b/src/include/access/session.h @@ -17,6 +17,9 @@ /* Avoid including typcache.h */ struct SharedRecordTypmodRegistry; +/* Avoid including undolog.h */ +struct UndoLogSlot; + /* * A struct encapsulating some elements of a user's session. For now this * manages state that applies to parallel query, but it principle it could @@ -27,6 +30,10 @@ typedef struct Session dsm_segment *segment; /* The session-scoped DSM segment. */ dsa_area *area; /* The session-scoped DSA area. */ + /* State managed by undolog.c. */ + struct UndoLogSlot *attached_undo_slots[4]; /* UndoLogCategories */ + bool need_to_choose_undo_tablespace; + Should we change 4 to UndoLogCategories or suitable macro? b) +static inline size_t +UndoLogNumSlots(void) +{ + return MaxBackends * 4; +} Should we change 4 to UndoLogCategories or suitable macro c) +allocate_empty_undo_segment(UndoLogNumber logno, Oid tablespace, + UndoLogOffset end) +{ + struct stat stat_buffer; + off_t size; + char path[MAXPGPATH]; + void *zeroes; + size_t nzeroes = 8192; + int fd; should we use BLCKSZ instead of 8192? 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? Regards, Vignesh EnterpriseDB: http://www.enterprisedb.com On Wed, Jul 24, 2019 at 5:44 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Jul 24, 2019 at 2:45 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Thu, Jul 18, 2019 at 5:10 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > 7. > > +attach_undo_log(UndoLogCategory category, Oid tablespace) > > { > > .. > > if (candidate->meta.tablespace == tablespace) > > + { > > + logno = *place; > > + slot = candidate; > > + *place = candidate->next_free; > > + break; > > + } > > > > Here, the code is breaking from the loop, so why do we need to set > > *place? Am I missing something obvious? > > > > I think I know what I was missing. It seems here you are removing an > element from the freelist. > > One point related to detach_current_undo_log. > > + LWLockAcquire(&slot->mutex, LW_EXCLUSIVE); > + slot->pid = InvalidPid; > + slot->meta.unlogged.xid = InvalidTransactionId; > + if (full) > + slot->meta.status = UNDO_LOG_STATUS_FULL; > + LWLockRelease(&slot->mutex); > > If I read the comments in structure UndoLogMetaData, it is mentioned > that 'status' is changed by explicit WAL record whereas there is no > WAL record in code to change the status. I see the problem as well if > we don't WAL log this change. Suppose after changing the status of > this log, we allocate a new log and insert some records in that log as > well for the same transaction for which we have inserted records in > the log which we just marked as FULL. Now, here we form the link > between two logs as the same transaction has overflowed into a new > log. Say, we crash after this. Now, after recovery the log won't be > marked as FULL which means there is a chance that it can be used for > some other transaction, if that happens, then our link for a > transaction spanning to different log will break and we won't be able > to access the data in another log. In short, I think it is important > to WAL log this status change unless I am missing something. > > -- > With Regards, > Amit Kapila. > EnterpriseDB: http://www.enterprisedb.com > > -- Regards, vignesh Have a nice day