public inbox for [email protected]
help / color / mirror / Atom feedIn-placre persistance change of a relation
52+ messages / 9 participants
[nested] [flat]
* In-placre persistance change of a relation
@ 2020-11-11 08:33 Kyotaro Horiguchi <[email protected]>
0 siblings, 2 replies; 52+ messages in thread
From: Kyotaro Horiguchi @ 2020-11-11 08:33 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Hello. This is a thread for an alternative solution to wal_level=none
[*1] for bulk data loading.
*1: https://www.postgresql.org/message-id/TYAPR01MB29901EBE5A3ACCE55BA99186FE320%40TYAPR01MB2990.jpnprd0...
At Tue, 10 Nov 2020 09:33:12 -0500, Stephen Frost <[email protected]> wrote in
> Greetings,
>
> * Kyotaro Horiguchi ([email protected]) wrote:
> > For fuel(?) of the discussion, I tried a very-quick PoC for in-place
> > ALTER TABLE SET LOGGED/UNLOGGED and resulted as attached. After some
> > trials of several ways, I drifted to the following way after poking
> > several ways.
> >
> > 1. Flip BM_PERMANENT of active buffers
> > 2. adding/removing init fork
> > 3. sync files,
> > 4. Flip pg_class.relpersistence.
> >
> > It always skips table copy in the SET UNLOGGED case, and only when
> > wal_level=minimal in the SET LOGGED case. Crash recovery seems
> > working by some brief testing by hand.
>
> Somehow missed that this patch more-or-less does what I was referring to
> down-thread, but I did want to mention that it looks like it's missing a
> necessary FlushRelationBuffers() call before the sync, otherwise there
> could be dirty buffers for the relation that's being set to LOGGED (with
> wal_level=minimal), which wouldn't be good. See the comments above
> smgrimmedsync().
Right. Thanks. However, since SetRelFileNodeBuffersPersistence()
called just above scans shared buffers so I don't want to just call
FlushRelationBuffers() separately. Instead, I added buffer-flush to
SetRelFileNodeBuffersPersistence().
FWIW this is a revised version of the PoC, which has some known
problems.
- Flipping of Buffer persistence is not WAL-logged nor even be able to
be safely roll-backed. (It might be better to drop buffers).
- This version handles indexes but not yet handle toast relatins.
- tableAMs are supposed to support this feature. (but I'm not sure
it's worth allowing them not to do so).
> > Of course, I haven't performed intensive test on it.
>
> Reading through the thread, it didn't seem very clear, but we should
> definitely make sure that it does the right thing on replicas when going
> between unlogged and logged (and between logged and unlogged too), of
> course.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
[text/x-patch] PoC_in-place_set_persistence_v2.patch (23.8K, ../../[email protected]/2-PoC_in-place_set_persistence_v2.patch)
download | inline diff:
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index dcaea7135f..0c6ce70484 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -613,6 +613,27 @@ heapam_relation_set_new_filenode(Relation rel,
smgrclose(srel);
}
+static void
+heapam_relation_set_persistence(Relation rel, char persistence)
+{
+ Assert(rel->rd_rel->relpersistence == RELPERSISTENCE_PERMANENT ||
+ rel->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED);
+
+ Assert (rel->rd_rel->relpersistence != persistence);
+
+ if (persistence == RELPERSISTENCE_UNLOGGED)
+ {
+ Assert(rel->rd_rel->relkind == RELKIND_RELATION ||
+ rel->rd_rel->relkind == RELKIND_MATVIEW ||
+ rel->rd_rel->relkind == RELKIND_TOASTVALUE);
+
+ RelationCreateInitFork(rel->rd_node, false);
+ }
+ else
+ RelationDropInitFork(rel->rd_node);
+}
+
+
static void
heapam_relation_nontransactional_truncate(Relation rel)
{
@@ -2540,6 +2561,7 @@ static const TableAmRoutine heapam_methods = {
.compute_xid_horizon_for_tuples = heap_compute_xid_horizon_for_tuples,
.relation_set_new_filenode = heapam_relation_set_new_filenode,
+ .relation_set_persistence = heapam_relation_set_persistence,
.relation_nontransactional_truncate = heapam_relation_nontransactional_truncate,
.relation_copy_data = heapam_relation_copy_data,
.relation_copy_for_cluster = heapam_relation_copy_for_cluster,
diff --git a/src/backend/access/rmgrdesc/smgrdesc.c b/src/backend/access/rmgrdesc/smgrdesc.c
index a7c0cb1bc3..8397002613 100644
--- a/src/backend/access/rmgrdesc/smgrdesc.c
+++ b/src/backend/access/rmgrdesc/smgrdesc.c
@@ -40,6 +40,14 @@ smgr_desc(StringInfo buf, XLogReaderState *record)
xlrec->blkno, xlrec->flags);
pfree(path);
}
+ else if (info == XLOG_SMGR_UNLINK)
+ {
+ xl_smgr_unlink *xlrec = (xl_smgr_unlink *) rec;
+ char *path = relpathperm(xlrec->rnode, xlrec->forkNum);
+
+ appendStringInfoString(buf, path);
+ pfree(path);
+ }
}
const char *
@@ -55,6 +63,9 @@ smgr_identify(uint8 info)
case XLOG_SMGR_TRUNCATE:
id = "TRUNCATE";
break;
+ case XLOG_SMGR_UNLINK:
+ id = "UNLINK";
+ break;
}
return id;
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index d538f25726..ac5aea3d38 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -60,6 +60,8 @@ int wal_skip_threshold = 2048; /* in kilobytes */
typedef struct PendingRelDelete
{
RelFileNode relnode; /* relation that may need to be deleted */
+ bool deleteinitfork; /* delete only init fork if true */
+ bool createinitfork; /* create init fork if true */
BackendId backend; /* InvalidBackendId if not a temp rel */
bool atCommit; /* T=delete at commit; F=delete at abort */
int nestLevel; /* xact nesting level of request */
@@ -153,6 +155,8 @@ RelationCreateStorage(RelFileNode rnode, char relpersistence)
pending = (PendingRelDelete *)
MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
pending->relnode = rnode;
+ pending->deleteinitfork = false;
+ pending->createinitfork = false;
pending->backend = backend;
pending->atCommit = false; /* delete if abort */
pending->nestLevel = GetCurrentTransactionNestLevel();
@@ -168,6 +172,95 @@ RelationCreateStorage(RelFileNode rnode, char relpersistence)
return srel;
}
+/*
+ * RelationCreateInitFork
+ * Create physical storage for a relation.
+ *
+ * Create the underlying disk file storage for the relation. This only
+ * creates the main fork; additional forks are created lazily by the
+ * modules that need them.
+ *
+ * This function is transactional. The creation is WAL-logged, and if the
+ * transaction aborts later on, the storage will be destroyed.
+ */
+void
+RelationCreateInitFork(RelFileNode rnode, bool isRedo)
+{
+ PendingRelDelete *pending;
+ SMgrRelation srel;
+ PendingRelDelete *prev;
+ PendingRelDelete *next;
+
+ prev = NULL;
+ for (pending = pendingDeletes; pending != NULL; pending = next)
+ {
+ next = pending->next;
+ if (RelFileNodeEquals(rnode, pending->relnode) &&
+ pending->deleteinitfork && pending->atCommit)
+ {
+ /* unlink and delete list entry */
+ if (prev)
+ prev->next = next;
+ else
+ pendingDeletes = next;
+ pfree(pending);
+ return;
+ }
+ else
+ {
+ /* unrelated entry, don't touch it */
+ prev = pending;
+ }
+ }
+ srel = smgropen(rnode, InvalidBackendId);
+ smgrcreate(srel, INIT_FORKNUM, isRedo);
+ if (!isRedo)
+ log_smgrcreate(&rnode, INIT_FORKNUM);
+ smgrimmedsync(srel, INIT_FORKNUM);
+
+ /* Add the relation to the list of stuff to delete at abort */
+ pending = (PendingRelDelete *)
+ MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
+ pending->relnode = rnode;
+ pending->deleteinitfork = true;
+ pending->createinitfork = false;
+ pending->backend = InvalidBackendId;
+ pending->atCommit = false; /* delete if abort */
+ pending->nestLevel = GetCurrentTransactionNestLevel();
+ pending->next = pendingDeletes;
+ pendingDeletes = pending;
+}
+
+void
+RelationDropInitFork(RelFileNode rnode)
+{
+ PendingRelDelete *pending;
+ PendingRelDelete *next;
+
+ for (pending = pendingDeletes; pending != NULL; pending = next)
+ {
+ next = pending->next;
+ if (RelFileNodeEquals(rnode, pending->relnode) &&
+ pending->deleteinitfork && pending->atCommit)
+ {
+ /* We're done. */
+ return;
+ }
+ }
+
+ /* Add the relation to the list of stuff to delete at abort */
+ pending = (PendingRelDelete *)
+ MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
+ pending->relnode = rnode;
+ pending->deleteinitfork = true;
+ pending->createinitfork = false;
+ pending->backend = InvalidBackendId;
+ pending->atCommit = true; /* create if abort */
+ pending->nestLevel = GetCurrentTransactionNestLevel();
+ pending->next = pendingDeletes;
+ pendingDeletes = pending;
+}
+
/*
* Perform XLogInsert of an XLOG_SMGR_CREATE record to WAL.
*/
@@ -187,6 +280,25 @@ log_smgrcreate(const RelFileNode *rnode, ForkNumber forkNum)
XLogInsert(RM_SMGR_ID, XLOG_SMGR_CREATE | XLR_SPECIAL_REL_UPDATE);
}
+/*
+ * Perform XLogInsert of an XLOG_SMGR_UNLINK record to WAL.
+ */
+void
+log_smgrunlink(const RelFileNode *rnode, ForkNumber forkNum)
+{
+ xl_smgr_unlink xlrec;
+
+ /*
+ * Make an XLOG entry reporting the file unlink.
+ */
+ xlrec.rnode = *rnode;
+ xlrec.forkNum = forkNum;
+
+ XLogBeginInsert();
+ XLogRegisterData((char *) &xlrec, sizeof(xlrec));
+ XLogInsert(RM_SMGR_ID, XLOG_SMGR_UNLINK | XLR_SPECIAL_REL_UPDATE);
+}
+
/*
* RelationDropStorage
* Schedule unlinking of physical storage at transaction commit.
@@ -200,6 +312,8 @@ RelationDropStorage(Relation rel)
pending = (PendingRelDelete *)
MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
pending->relnode = rel->rd_node;
+ pending->createinitfork = false;
+ pending->deleteinitfork = false;
pending->backend = rel->rd_backend;
pending->atCommit = true; /* delete if commit */
pending->nestLevel = GetCurrentTransactionNestLevel();
@@ -626,19 +740,27 @@ smgrDoPendingDeletes(bool isCommit)
srel = smgropen(pending->relnode, pending->backend);
- /* allocate the initial array, or extend it, if needed */
- if (maxrels == 0)
+ if (pending->deleteinitfork)
{
- maxrels = 8;
- srels = palloc(sizeof(SMgrRelation) * maxrels);
+ log_smgrunlink(&pending->relnode, INIT_FORKNUM);
+ smgrunlink(srel, INIT_FORKNUM, false);
}
- else if (maxrels <= nrels)
+ else
{
- maxrels *= 2;
- srels = repalloc(srels, sizeof(SMgrRelation) * maxrels);
- }
+ /* allocate the initial array, or extend it, if needed */
+ if (maxrels == 0)
+ {
+ maxrels = 8;
+ srels = palloc(sizeof(SMgrRelation) * maxrels);
+ }
+ else if (maxrels <= nrels)
+ {
+ maxrels *= 2;
+ srels = repalloc(srels, sizeof(SMgrRelation) * maxrels);
+ }
- srels[nrels++] = srel;
+ srels[nrels++] = srel;
+ }
}
/* must explicitly free the list entry */
pfree(pending);
@@ -917,6 +1039,14 @@ smgr_redo(XLogReaderState *record)
reln = smgropen(xlrec->rnode, InvalidBackendId);
smgrcreate(reln, xlrec->forkNum, true);
}
+ else if (info == XLOG_SMGR_UNLINK)
+ {
+ xl_smgr_unlink *xlrec = (xl_smgr_unlink *) XLogRecGetData(record);
+ SMgrRelation reln;
+
+ reln = smgropen(xlrec->rnode, InvalidBackendId);
+ smgrunlink(reln, xlrec->forkNum, true);
+ }
else if (info == XLOG_SMGR_TRUNCATE)
{
xl_smgr_truncate *xlrec = (xl_smgr_truncate *) XLogRecGetData(record);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e3cfaf8b07..e358174b01 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4918,6 +4918,137 @@ ATParseTransformCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
return newcmd;
}
+static bool
+try_inplace_persistence_change(AlteredTableInfo *tab, char persistence,
+ LOCKMODE lockmode)
+{
+ Relation rel;
+ Relation classRel;
+ HeapTuple tuple,
+ newtuple;
+ Datum new_val[Natts_pg_class];
+ bool new_null[Natts_pg_class],
+ new_repl[Natts_pg_class];
+ int i;
+ List *relids;
+ ListCell *lc_oid;
+
+ Assert(tab->rewrite == AT_REWRITE_ALTER_PERSISTENCE);
+ Assert(lockmode == AccessExclusiveLock);
+
+ /*
+ * Under the following condition, we need to call ATRewriteTable, which
+ * cannot be false in the AT_REWRITE_ALTER_PERSISTENCE case.
+ */
+ Assert(tab->constraints == NULL && tab->partition_constraint == NULL &&
+ tab->newvals == NULL && !tab->verify_new_notnull);
+
+ /*
+ * When wal_level is replica or higher we need that the initial state of
+ * the relation be recoverable from WAL. When wal_level >= replica
+ * switching to PERMANENT needs to emit the WAL records to reconstruct the
+ * current data. This could be done by writing XLOG_FPI for all pages but
+ * it is not obvious that that is performant than normal rewriting.
+ * Otherwise what we need for the relation data is just establishing
+ * initial state on storage and no need of WAL to reconstruct it.
+ */
+ if (tab->newrelpersistence == RELPERSISTENCE_PERMANENT && XLogIsNeeded())
+ return false;
+
+ rel = table_open(tab->relid, lockmode);
+
+ Assert(rel->rd_rel->relpersistence != persistence);
+
+ elog(DEBUG1, "perform im-place persistnce change");
+
+ RelationOpenSmgr(rel);
+
+ /* Change persistence then flush-out buffers of the relation */
+
+ /* Get the list of index OIDs for this relation */
+ relids = RelationGetIndexList(rel);
+ relids = lcons_oid(rel->rd_id, relids);
+
+ table_close(rel, lockmode);
+
+ /* Done change on storage. Update catalog including indexes. */
+ /* add the heap oid to the relation ID list */
+
+ classRel = table_open(RelationRelationId, RowExclusiveLock);
+
+ foreach (lc_oid, relids)
+ {
+ Oid reloid = lfirst_oid(lc_oid);
+ Relation r = relation_open(reloid, lockmode);
+
+ RelationOpenSmgr(r);
+
+ if (persistence == RELPERSISTENCE_UNLOGGED)
+ {
+ RelationCreateInitFork(r->rd_node, false);
+
+ if (r->rd_rel->relkind == RELKIND_INDEX ||
+ r->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+ r->rd_indam->ambuildempty(r);
+ else
+ {
+ Assert(r->rd_rel->relkind == RELKIND_RELATION ||
+ r->rd_rel->relkind == RELKIND_MATVIEW ||
+ r->rd_rel->relkind == RELKIND_TOASTVALUE);
+ }
+ }
+ else
+ RelationDropInitFork(r->rd_node);
+
+ table_close(r, NoLock);
+
+ /*
+ * This relation is now WAL-logged. Sync all files immediately to
+ * establish the initial state on storgae.
+ */
+ if (persistence == RELPERSISTENCE_PERMANENT)
+ {
+ for (i = 0 ; i < MAX_FORKNUM ; i++)
+ {
+ if (smgrexists(r->rd_smgr, i))
+ smgrimmedsync(r->rd_smgr, i);
+ }
+ }
+
+
+ tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(reloid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for relation %u", reloid);
+
+ memset(new_val, 0, sizeof(new_val));
+ memset(new_null, false, sizeof(new_null));
+ memset(new_repl, false, sizeof(new_repl));
+
+ new_val[Anum_pg_class_relpersistence - 1] = CharGetDatum(persistence);
+ new_null[Anum_pg_class_relpersistence - 1] = false;
+ new_repl[Anum_pg_class_relpersistence - 1] = true;
+
+ newtuple = heap_modify_tuple(tuple, RelationGetDescr(classRel),
+ new_val, new_null, new_repl);
+
+ CatalogTupleUpdate(classRel, &newtuple->t_self, newtuple);
+ heap_freetuple(newtuple);
+ }
+
+ foreach (lc_oid, relids)
+ {
+ Oid reloid = lfirst_oid(lc_oid);
+ Relation r = relation_open(reloid, lockmode);
+
+ RelationOpenSmgr(r);
+ SetRelationBuffersPersistence(r, persistence == RELPERSISTENCE_PERMANENT);
+ table_close(r, NoLock);
+ }
+ table_close(classRel, RowExclusiveLock);
+
+ return true;
+}
+
/*
* ATRewriteTables: ALTER TABLE phase 3
*/
@@ -5038,45 +5169,51 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
tab->relid,
tab->rewrite);
- /*
- * Create transient table that will receive the modified data.
- *
- * Ensure it is marked correctly as logged or unlogged. We have
- * to do this here so that buffers for the new relfilenode will
- * have the right persistence set, and at the same time ensure
- * that the original filenode's buffers will get read in with the
- * correct setting (i.e. the original one). Otherwise a rollback
- * after the rewrite would possibly result with buffers for the
- * original filenode having the wrong persistence setting.
- *
- * NB: This relies on swap_relation_files() also swapping the
- * persistence. That wouldn't work for pg_class, but that can't be
- * unlogged anyway.
- */
- OIDNewHeap = make_new_heap(tab->relid, NewTableSpace, persistence,
- lockmode);
+ if (tab->rewrite != AT_REWRITE_ALTER_PERSISTENCE ||
+ !try_inplace_persistence_change(tab, persistence, lockmode))
+ {
+ /*
+ * Create transient table that will receive the modified data.
+ *
+ * Ensure it is marked correctly as logged or unlogged. We
+ * have to do this here so that buffers for the new relfilenode
+ * will have the right persistence set, and at the same time
+ * ensure that the original filenode's buffers will get read in
+ * with the correct setting (i.e. the original one). Otherwise
+ * a rollback after the rewrite would possibly result with
+ * buffers for the original filenode having the wrong
+ * persistence setting.
+ *
+ * NB: This relies on swap_relation_files() also swapping the
+ * persistence. That wouldn't work for pg_class, but that can't
+ * be unlogged anyway.
+ */
+ OIDNewHeap = make_new_heap(tab->relid, NewTableSpace, persistence,
+ lockmode);
- /*
- * Copy the heap data into the new table with the desired
- * modifications, and test the current data within the table
- * against new constraints generated by ALTER TABLE commands.
- */
- ATRewriteTable(tab, OIDNewHeap, lockmode);
+ /*
+ * Copy the heap data into the new table with the desired
+ * modifications, and test the current data within the table
+ * against new constraints generated by ALTER TABLE commands.
+ */
+ ATRewriteTable(tab, OIDNewHeap, lockmode);
- /*
- * Swap the physical files of the old and new heaps, then rebuild
- * indexes and discard the old heap. We can use RecentXmin for
- * the table's new relfrozenxid because we rewrote all the tuples
- * in ATRewriteTable, so no older Xid remains in the table. Also,
- * we never try to swap toast tables by content, since we have no
- * interest in letting this code work on system catalogs.
- */
- finish_heap_swap(tab->relid, OIDNewHeap,
- false, false, true,
- !OidIsValid(tab->newTableSpace),
- RecentXmin,
- ReadNextMultiXactId(),
- persistence);
+ /*
+ * Swap the physical files of the old and new heaps, then
+ * rebuild indexes and discard the old heap. We can use
+ * RecentXmin for the table's new relfrozenxid because we
+ * rewrote all the tuples in ATRewriteTable, so no older Xid
+ * remains in the table. Also, we never try to swap toast
+ * tables by content, since we have no interest in letting this
+ * code work on system catalogs.
+ */
+ finish_heap_swap(tab->relid, OIDNewHeap,
+ false, false, true,
+ !OidIsValid(tab->newTableSpace),
+ RecentXmin,
+ ReadNextMultiXactId(),
+ persistence);
+ }
}
else
{
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index ad0d1a9abc..c71e1a5f92 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3033,6 +3033,80 @@ DropRelFileNodeBuffers(RelFileNodeBackend rnode, ForkNumber *forkNum,
}
}
+/* ---------------------------------------------------------------------
+ * SetRelFileNodeBuffersPersistence
+ *
+ * This function changes the persistence of all buffer pages of a relation
+ * then writes all dirty pages of the relation out to disk when switching
+ * to PERMANENT. (or more accurately, out to kernel disk buffers),
+ * ensuring that the kernel has an up-to-date view of the relation.
+ *
+ * Generally, the caller should be holding AccessExclusiveLock on the
+ * target relation to ensure that no other backend is busy dirtying
+ * more blocks of the relation; the effects can't be expected to last
+ * after the lock is released.
+ *
+ * XXX currently it sequentially searches the buffer pool, should be
+ * changed to more clever ways of searching. This routine is not
+ * used in any performance-critical code paths, so it's not worth
+ * adding additional overhead to normal paths to make it go faster;
+ * but see also DropRelFileNodeBuffers.
+ * --------------------------------------------------------------------
+ */
+void
+SetRelationBuffersPersistence(Relation rel, bool permanent)
+{
+ int i;
+ RelFileNodeBackend rnode = rel->rd_smgr->smgr_rnode;
+
+ Assert (!RelFileNodeBackendIsTemp(rnode));
+
+ for (i = 0; i < NBuffers; i++)
+ {
+ BufferDesc *bufHdr = GetBufferDescriptor(i);
+ uint32 buf_state;
+
+ if (!RelFileNodeEquals(bufHdr->tag.rnode, rnode.node))
+ continue;
+
+ ReservePrivateRefCountEntry();
+
+ buf_state = LockBufHdr(bufHdr);
+
+ if (RelFileNodeEquals(bufHdr->tag.rnode, rnode.node))
+ {
+ ereport(LOG, (errmsg ("#%d: %d", i, (buf_state & BM_PERMANENT) == 0), errhidestmt(true)));
+ if (permanent)
+ {
+ Assert ((buf_state & BM_PERMANENT) == 0);
+ buf_state |= BM_PERMANENT;
+ pg_atomic_write_u32(&bufHdr->state, buf_state);
+
+ /* we flush this buffer when swithing to PERMANENT */
+ if ((buf_state & (BM_VALID | BM_DIRTY)) ==
+ (BM_VALID | BM_DIRTY))
+ {
+ PinBuffer_Locked(bufHdr);
+ LWLockAcquire(BufferDescriptorGetContentLock(bufHdr),
+ LW_SHARED);
+ FlushBuffer(bufHdr, rel->rd_smgr);
+ LWLockRelease(BufferDescriptorGetContentLock(bufHdr));
+ UnpinBuffer(bufHdr, true);
+ }
+ else
+ UnlockBufHdr(bufHdr, buf_state);
+ }
+ else
+ {
+ Assert ((buf_state & BM_PERMANENT) != 0);
+ buf_state &= ~BM_PERMANENT;
+ UnlockBufHdr(bufHdr, buf_state);
+ }
+ ereport(LOG, (errmsg ("#%d: -> %d", i, (buf_state & BM_PERMANENT) == 0), errhidestmt(true)));
+ }
+ }
+}
+
/* ---------------------------------------------------------------------
* DropRelFileNodesAllBuffers
*
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index dcc09df0c7..5eb9e97b3d 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -645,6 +645,12 @@ smgrimmedsync(SMgrRelation reln, ForkNumber forknum)
smgrsw[reln->smgr_which].smgr_immedsync(reln, forknum);
}
+void
+smgrunlink(SMgrRelation reln, ForkNumber forknum, bool isRedo)
+{
+ smgrsw[reln->smgr_which].smgr_unlink(reln->smgr_rnode, forknum, isRedo);
+}
+
/*
* AtEOXact_SMgr
*
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 387eb34a61..1d19278a18 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -451,6 +451,15 @@ typedef struct TableAmRoutine
TransactionId *freezeXid,
MultiXactId *minmulti);
+ /*
+ * This callback needs to switch persistence of the relation between
+ * RELPERSISTENCE_PERMANENT and RELPERSISTENCE_UNLOGGED. Actual change on
+ * storage is performed elsewhere.
+ *
+ * See also table_relation_set_persistence().
+ */
+ void (*relation_set_persistence) (Relation rel, char persistence);
+
/*
* This callback needs to remove all contents from `rel`'s current
* relfilenode. No provisions for transactional behaviour need to be made.
@@ -1404,6 +1413,18 @@ table_relation_set_new_filenode(Relation rel,
freezeXid, minmulti);
}
+/*
+ * Switch storage persistence between RELPERSISTENCE_PERMANENT and
+ * RELPERSISTENCE_UNLOGGED.
+ *
+ * This is used during in-place persistence switching
+ */
+static inline void
+table_relation_set_persistence(Relation rel, char persistence)
+{
+ rel->rd_tableam->relation_set_persistence(rel, persistence);
+}
+
/*
* Remove all table contents from `rel`, in a non-transactional manner.
* Non-transactional meaning that there's no need to support rollbacks. This
diff --git a/src/include/catalog/storage.h b/src/include/catalog/storage.h
index 30c38e0ca6..43d2eb0fb4 100644
--- a/src/include/catalog/storage.h
+++ b/src/include/catalog/storage.h
@@ -23,6 +23,8 @@
extern int wal_skip_threshold;
extern SMgrRelation RelationCreateStorage(RelFileNode rnode, char relpersistence);
+extern void RelationCreateInitFork(RelFileNode rel, bool isRedo);
+extern void RelationDropInitFork(RelFileNode rel);
extern void RelationDropStorage(Relation rel);
extern void RelationPreserveStorage(RelFileNode rnode, bool atCommit);
extern void RelationPreTruncate(Relation rel);
diff --git a/src/include/catalog/storage_xlog.h b/src/include/catalog/storage_xlog.h
index 7b21cab2e0..73ad2ae89e 100644
--- a/src/include/catalog/storage_xlog.h
+++ b/src/include/catalog/storage_xlog.h
@@ -29,6 +29,7 @@
/* XLOG gives us high 4 bits */
#define XLOG_SMGR_CREATE 0x10
#define XLOG_SMGR_TRUNCATE 0x20
+#define XLOG_SMGR_UNLINK 0x30
typedef struct xl_smgr_create
{
@@ -36,6 +37,12 @@ typedef struct xl_smgr_create
ForkNumber forkNum;
} xl_smgr_create;
+typedef struct xl_smgr_unlink
+{
+ RelFileNode rnode;
+ ForkNumber forkNum;
+} xl_smgr_unlink;
+
/* flags for xl_smgr_truncate */
#define SMGR_TRUNCATE_HEAP 0x0001
#define SMGR_TRUNCATE_VM 0x0002
@@ -51,6 +58,7 @@ typedef struct xl_smgr_truncate
} xl_smgr_truncate;
extern void log_smgrcreate(const RelFileNode *rnode, ForkNumber forkNum);
+extern void log_smgrunlink(const RelFileNode *rnode, ForkNumber forkNum);
extern void smgr_redo(XLogReaderState *record);
extern void smgr_desc(StringInfo buf, XLogReaderState *record);
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index ee91b8fa26..f65a273999 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -205,6 +205,7 @@ extern void FlushRelationsAllBuffers(struct SMgrRelationData **smgrs, int nrels)
extern void FlushDatabaseBuffers(Oid dbid);
extern void DropRelFileNodeBuffers(RelFileNodeBackend rnode, ForkNumber *forkNum,
int nforks, BlockNumber *firstDelBlock);
+extern void SetRelationBuffersPersistence(Relation rnode, bool permanent);
extern void DropRelFileNodesAllBuffers(RelFileNodeBackend *rnodes, int nnodes);
extern void DropDatabaseBuffers(Oid dbid);
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index f28a842401..5d74631006 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -86,6 +86,7 @@ extern void smgrclose(SMgrRelation reln);
extern void smgrcloseall(void);
extern void smgrclosenode(RelFileNodeBackend rnode);
extern void smgrcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo);
+extern void smgrunlink(SMgrRelation reln, ForkNumber forknum, bool isRedo);
extern void smgrdosyncall(SMgrRelation *rels, int nrels);
extern void smgrdounlinkall(SMgrRelation *rels, int nrels, bool isRedo);
extern void smgrextend(SMgrRelation reln, ForkNumber forknum,
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2020-11-11 22:18 Andres Freund <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
1 sibling, 1 reply; 52+ messages in thread
From: Andres Freund @ 2020-11-11 22:18 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Hi,
I suggest outlining what you are trying to achieve here. Starting a new
thread and expecting people to dig through another thread to infer what
you are actually trying to achive isn't great.
FWIW, I'm *extremely* doubtful it's worth adding features that depend on
a PGC_POSTMASTER wal_level=minimal being used. Which this does, a far as
I understand. If somebody added support for dynamically adapting
wal_level (e.g. wal_level=auto, that increases wal_level to
replica/logical depending on the presence of replication slots), it'd
perhaps be different.
On 2020-11-11 17:33:17 +0900, Kyotaro Horiguchi wrote:
> FWIW this is a revised version of the PoC, which has some known
> problems.
>
> - Flipping of Buffer persistence is not WAL-logged nor even be able to
> be safely roll-backed. (It might be better to drop buffers).
That's obviously a no-go. I think you might be able to address this if
you accept that the command cannot be run in a transaction (like
CONCURRENTLY). Then you can first do the catalog changes, change the
persistence level, and commit.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2020-11-12 06:55 Kyotaro Horiguchi <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 0 replies; 52+ messages in thread
From: Kyotaro Horiguchi @ 2020-11-12 06:55 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Wed, 11 Nov 2020 14:18:04 -0800, Andres Freund <[email protected]> wrote in
> Hi,
>
> I suggest outlining what you are trying to achieve here. Starting a new
> thread and expecting people to dig through another thread to infer what
> you are actually trying to achive isn't great.
Agreed. I'll post that. Thanks.
> FWIW, I'm *extremely* doubtful it's worth adding features that depend on
> a PGC_POSTMASTER wal_level=minimal being used. Which this does, a far as
> I understand. If somebody added support for dynamically adapting
> wal_level (e.g. wal_level=auto, that increases wal_level to
> replica/logical depending on the presence of replication slots), it'd
> perhaps be different.
Yes, this depends on wal_level=minimal for switching from UNLOGGED to
LOGGED, that's similar to COPY/INSERT-to-intransaction-created-tables
optimization for wal_level=minimal. And it expands that optimization
to COPY/INSERT-to-existent-tables, which seems worth doing.
Switching to LOGGED needs to emit the initial state to WAL... Hmm.. I
came to think that even in that case skipping table copy reduces I/O
significantly, even though FPI-WAL is emitted.
> On 2020-11-11 17:33:17 +0900, Kyotaro Horiguchi wrote:
> > FWIW this is a revised version of the PoC, which has some known
> > problems.
> >
> > - Flipping of Buffer persistence is not WAL-logged nor even be able to
> > be safely roll-backed. (It might be better to drop buffers).
>
> That's obviously a no-go. I think you might be able to address this if
> you accept that the command cannot be run in a transaction (like
> CONCURRENTLY). Then you can first do the catalog changes, change the
> persistence level, and commit.
Of course. The next version reverts persistence change at abort.
Thanks!
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2020-11-13 04:22 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
1 sibling, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2020-11-13 04:22 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Hello. Before posting the next version, I'd like to explain what this
patch is.
1. The Issue
Bulk data loading is a long-time taking, I/O consuming task. Many
DBAs want that task is faster, even at the cost of increasing risk of
data-loss. wal_level=minimal is an answer to such a
request. Data-loading onto a table that is created in the current
transaction omits WAL-logging and synced at commit.
However, the optimization doesn't benefit the case where the
data-loading is performed onto existing tables. There are quite a few
cases where data is loaded into tables that already contains a lot of
data. Those cases don't take benefit of the optimization.
Another possible solution for bulk data-loading is UNLOGGED
tables. But when we switch LOGGED/UNLOGGED of a table, all the table
content is copied to a newly created heap, which is costly.
2. Proposed Solutions.
There are two proposed solutions are discussed on this mailing
list. One is wal_level = none (*1), which omits WAL-logging almost at
all. Another is extending the existing optimization to the ALTER TABLE
SET LOGGED/UNLOGGED cases, which is to be discussed in this new
thread.
3. In-place Persistence Change
So the attached is a PoC patch of the "another" solution. When we
want to change table persistence in-place, basically we need to do the
following steps.
(the talbe is exclusively locked)
(1) Flip BM_PERMANENT flag of all shared buffer blocks for the heap.
(2) Create or delete the init fork for existing heap.
(3) Flush all buffers of the relation to file system.
(4) Sync heap files.
(5) Make catalog changes.
4. Transactionality
The 1, 2 and 5 above need to be abort-able. 5 is rolled back by
existing infrastructure, and rolling-back of 1 and 2 are achieved by
piggybacking on the pendingDeletes mechanism.
5. Replication
Furthermore, that changes ought to be replicable to standbys. Catalog
changes are replicated as usual.
On-the-fly creation of the init fork leads to recovery mess. Even
though it is removed at abort, if the server crashed before
transaction end, the file is left alone and corrupts database in the
next recovery. I sought a way to create the init fork in
smgrPendingDelete but that needs relcache and relcache is not
available at that late of commit. Finally, I introduced the fifth fork
kind "INITTMP"(_itmp) only to signal that the init file is not
committed. I don't like that way but it seems working fine...
6. SQL Command
The second file in the patchset adds a syntax that changes persistence
of all tables in a tablespace.
ALTER TABLE ALL IN TABLESPACE <tsp> SET LOGGED/UNLOGGED [ NOWAIT ];
7. Testing
I tried to write TAP test for this, but IPC::Run::harness (or
interactive_psql) doesn't seem to work for me. I'm not sure what
exactly is happening but pty redirection doesn't work.
$in = "ls\n"; $out = ""; run ["/usr/bin/bash"], \$in, \$out; print $out;
works but
$in = "ls\n"; $out = ""; run ["/usr/bin/bash"], '<pty<', \$in, '>pty>', \$out; print $out;
doesn't respond.
The patch is attached.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* RE: In-placre persistance change of a relation
@ 2020-11-13 06:43 [email protected] <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 2 replies; 52+ messages in thread
From: [email protected] @ 2020-11-13 06:43 UTC (permalink / raw)
To: 'Kyotaro Horiguchi' <[email protected]>; +Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>
Hi Horiguchi-san,
Thank you for making a patch so quickly. I've started looking at it.
What makes you think this is a PoC? Documentation and test cases? If there's something you think that doesn't work or are concerned about, can you share it?
Do you know the reason why data copy was done before? And, it may be odd for me to ask this, but I think I saw someone referred to the past discussion that eliminating data copy is difficult due to some processing at commit. I can't find it.
(1)
@@ -168,6 +168,8 @@ extern PGDLLIMPORT int32 *LocalRefCount;
*/
#define BufferGetPage(buffer) ((Page)BufferGetBlock(buffer))
+struct SmgrRelationData;
This declaration is already in the file:
/* forward declared, to avoid having to expose buf_internals.h here */
struct WritebackContext;
/* forward declared, to avoid including smgr.h here */
struct SMgrRelationData;
Regards
Takayuki Tsunakawa
^ permalink raw reply [nested|flat] 52+ messages in thread
* RE: In-placre persistance change of a relation
@ 2020-11-13 07:15 [email protected] <[email protected]>
parent: [email protected] <[email protected]>
1 sibling, 1 reply; 52+ messages in thread
From: [email protected] @ 2020-11-13 07:15 UTC (permalink / raw)
To: [email protected] <[email protected]>; 'Kyotaro Horiguchi' <[email protected]>; +Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>
Hello, Tsunakawa-San
> Do you know the reason why data copy was done before? And, it may be
> odd for me to ask this, but I think I saw someone referred to the past
> discussion that eliminating data copy is difficult due to some processing at
> commit. I can't find it.
I can share 2 sources why to eliminate the data copy is difficult in hackers thread.
Tom's remark and the context to copy relation's data.
https://www.postgresql.org/message-id/flat/31724.1394163360%40sss.pgh.pa.us#[email protected]...
Amit-San quoted this thread and mentioned that point in another thread.
https://www.postgresql.org/message-id/CAA4eK1%2BHDqS%2B1fhs5Jf9o4ZujQT%3DXBZ6sU0kOuEh2hqQAC%2Bt%3Dw%...
Best,
Takamichi Osumi
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2020-11-13 07:47 Kyotaro Horiguchi <[email protected]>
parent: [email protected] <[email protected]>
1 sibling, 0 replies; 52+ messages in thread
From: Kyotaro Horiguchi @ 2020-11-13 07:47 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Fri, 13 Nov 2020 06:43:13 +0000, "[email protected]" <[email protected]> wrote in
> Hi Horiguchi-san,
>
>
> Thank you for making a patch so quickly. I've started looking at it.
>
> What makes you think this is a PoC? Documentation and test cases? If there's something you think that doesn't work or are concerned about, can you share it?
The latest version is heavily revised and is given much comment so it
might have exited from PoC state. The necessity of documentation is
doubtful since this patch doesn't user-facing behavior other than
speed. Some tests are required especialy about recovery and
replication perspective but I haven't been able to make it. (One of
the tests needs to cause crash while a transaction is running.)
> Do you know the reason why data copy was done before? And, it may be odd for me to ask this, but I think I saw someone referred to the past discussion that eliminating data copy is difficult due to some processing at commit. I can't find it.
To imagine that, just because it is simpler considering rollback and
code sharing, and maybe no one have been complained that SET
LOGGED/UNLOGGED looks taking a long time than required/expected.
The current implement is simple. It's enough to just discard old or
new relfilenode according to the current transaction ends with commit
or abort. Tweaking of relfilenode under use leads-in some skews in
some places. I used pendingDelete mechanism a bit complexified way
and a violated an abstraction (I think, calling AM-routines from
storage.c is not good.) and even introduce a new fork kind only to
mark a init fork as "not committed yet". There might be better way,
but I haven't find it.
(The patch scans all shared buffer blocks for each relation).
> (1)
> @@ -168,6 +168,8 @@ extern PGDLLIMPORT int32 *LocalRefCount;
> */
> #define BufferGetPage(buffer) ((Page)BufferGetBlock(buffer))
>
> +struct SmgrRelationData;
>
> This declaration is already in the file:
>
> /* forward declared, to avoid having to expose buf_internals.h here */
> struct WritebackContext;
>
> /* forward declared, to avoid including smgr.h here */
> struct SMgrRelationData;
Hmmm. Nice chatch. And will fix in the next version.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2020-11-13 08:23 Kyotaro Horiguchi <[email protected]>
parent: [email protected] <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2020-11-13 08:23 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Fri, 13 Nov 2020 07:15:41 +0000, "[email protected]" <[email protected]> wrote in
> Hello, Tsunakawa-San
>
Thanks for sharing it!
> > Do you know the reason why data copy was done before? And, it may be
> > odd for me to ask this, but I think I saw someone referred to the past
> > discussion that eliminating data copy is difficult due to some processing at
> > commit. I can't find it.
> I can share 2 sources why to eliminate the data copy is difficult in hackers thread.
>
> Tom's remark and the context to copy relation's data.
> https://www.postgresql.org/message-id/flat/31724.1394163360%40sss.pgh.pa.us#[email protected]...
https://www.postgresql.org/message-id/[email protected]...
> No, not really. The issue is more around what happens if we crash
> part way through. At crash recovery time, the system catalogs are not
> available, because the database isn't consistent yet and, anyway, the
> startup process can't be bound to a database, let alone every database
> that might contain unlogged tables. So the sentinel that's used to
> decide whether to flush the contents of a table or index is the
> presence or absence of an _init fork, which the startup process
> obviously can see just fine. The _init fork also tells us what to
> stick in the relation when we reset it; for a table, we can just reset
> to an empty file, but that's not legal for indexes, so the _init fork
> contains a pre-initialized empty index that we can just copy over.
>
> Now, to make an unlogged table logged, you've got to at some stage
> remove those _init forks. But this is not a transactional operation.
> If you remove the _init forks and then the transaction rolls back,
> you've left the system an inconsistent state. If you postpone the
> removal until commit time, then you have a problem if it fails,
It's true. That are the cause of headache.
> particularly if it works for the first file but fails for the second.
> And if you crash at any point before you've fsync'd the containing
> directory, you have no idea which files will still be on disk after a
> hard reboot.
This is not an issue in this patch *except* the case where init fork
is failed to removed but the following removal of inittmp fork
succeeds. Another idea is adding a "not-yet-committed" property to a
fork. I added a new fork type for easiness of the patch but I could
go that way if that is an issue.
> Amit-San quoted this thread and mentioned that point in another thread.
> https://www.postgresql.org/message-id/CAA4eK1%2BHDqS%2B1fhs5Jf9o4ZujQT%3DXBZ6sU0kOuEh2hqQAC%2Bt%3Dw%...
This sounds like a bit differrent discussion. Making part-of-a-table
UNLOGGED looks far difficult to me.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* RE: In-placre persistance change of a relation
@ 2020-12-04 07:49 [email protected] <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: [email protected] @ 2020-12-04 07:49 UTC (permalink / raw)
To: 'Kyotaro Horiguchi' <[email protected]>; +Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>
From: Kyotaro Horiguchi <[email protected]>
> > No, not really. The issue is more around what happens if we crash
> > part way through. At crash recovery time, the system catalogs are not
> > available, because the database isn't consistent yet and, anyway, the
> > startup process can't be bound to a database, let alone every database
> > that might contain unlogged tables. So the sentinel that's used to
> > decide whether to flush the contents of a table or index is the
> > presence or absence of an _init fork, which the startup process
> > obviously can see just fine. The _init fork also tells us what to
> > stick in the relation when we reset it; for a table, we can just reset
> > to an empty file, but that's not legal for indexes, so the _init fork
> > contains a pre-initialized empty index that we can just copy over.
> >
> > Now, to make an unlogged table logged, you've got to at some stage
> > remove those _init forks. But this is not a transactional operation.
> > If you remove the _init forks and then the transaction rolls back,
> > you've left the system an inconsistent state. If you postpone the
> > removal until commit time, then you have a problem if it fails,
>
> It's true. That are the cause of headache.
...
> The current implement is simple. It's enough to just discard old or
> new relfilenode according to the current transaction ends with commit
> or abort. Tweaking of relfilenode under use leads-in some skews in
> some places. I used pendingDelete mechanism a bit complexified way
> and a violated an abstraction (I think, calling AM-routines from
> storage.c is not good.) and even introduce a new fork kind only to
> mark a init fork as "not committed yet". There might be better way,
> but I haven't find it.
I have no alternative idea yet, too. I agree that we want to avoid them, especially introducing inittmp fork... Anyway, below are the rest of my review comments for 0001. I want to review 0002 when we have decided to go with 0001.
(2)
XLOG_SMGR_UNLINK seems to necessitate modification of the following comments:
[src/include/catalog/storage_xlog.h]
/*
* Declarations for smgr-related XLOG records
*
* Note: we log file creation and truncation here, but logging of deletion
* actions is handled by xact.c, because it is part of transaction commit.
*/
[src/backend/access/transam/README]
3. Deleting a table, which requires an unlink() that could fail.
Our approach here is to WAL-log the operation first, but to treat failure
of the actual unlink() call as a warning rather than error condition.
Again, this can leave an orphan file behind, but that's cheap compared to
the alternatives. Since we can't actually do the unlink() until after
we've committed the DROP TABLE transaction, throwing an error would be out
of the question anyway. (It may be worth noting that the WAL entry about
the file deletion is actually part of the commit record for the dropping
transaction.)
(3)
+/* This is bit-map, not ordianal numbers */
There seems to be no comments using "bit-map". "Flags for ..." can be seen here and there.
(4)
Some wrong spellings:
+ /* we flush this buffer when swithing to PERMANENT */
swithing -> switching
+ * alredy flushed out by RelationCreate(Drop)InitFork called just
alredy -> already
+ * relation content to be WAL-logged to recovery the table.
recovery -> recover
+ * The inittmp fork works as the sentinel to identify that situaton.
situaton -> situation
(5)
+ table_close(classRel, NoLock);
+
+
+
+
}
These empty lines can be deleted.
(6)
+/*
+ * Perform XLogInsert of an XLOG_SMGR_UNLINK record to WAL.
+ */
+void
+log_smgrbufpersistence(const RelFileNode *rnode, bool persistence)
...
+ * Make an XLOG entry reporting the file unlink.
Not unlink but buffer persistence?
(7)
+ /*
+ * index-init fork needs further initialization. ambuildempty shoud do
+ * WAL-log and file sync by itself but otherwise we do that by myself.
+ */
+ if (rel->rd_rel->relkind == RELKIND_INDEX)
+ rel->rd_indam->ambuildempty(rel);
+ else
+ {
+ log_smgrcreate(&rnode, INIT_FORKNUM);
+ smgrimmedsync(srel, INIT_FORKNUM);
+ }
+
+ /*
+ * We have created the init fork. If server crashes before the current
+ * transaction ends the init fork left alone corrupts data while recovery.
+ * The inittmp fork works as the sentinel to identify that situaton.
+ */
+ smgrcreate(srel, INITTMP_FORKNUM, false);
+ log_smgrcreate(&rnode, INITTMP_FORKNUM);
+ smgrimmedsync(srel, INITTMP_FORKNUM);
If the server crashes between these two processings, only the init fork exists. Is it correct to create the inittmp fork first?
(8)
+ if (inxact_created)
+ {
+ SMgrRelation srel = smgropen(rnode, InvalidBackendId);
+ smgrclose(srel);
+ log_smgrunlink(&rnode, INIT_FORKNUM);
+ smgrunlink(srel, INIT_FORKNUM, false);
+ log_smgrunlink(&rnode, INITTMP_FORKNUM);
+ smgrunlink(srel, INITTMP_FORKNUM, false);
+ return;
+ }
smgrclose() should be called just before return.
Isn't it necessary here to revert buffer persistence state change?
(9)
+void
+smgrunlink(SMgrRelation reln, ForkNumber forknum, bool isRedo)
+{
+ smgrsw[reln->smgr_which].smgr_unlink(reln->smgr_rnode, forknum, isRedo);
+}
Maybe it's better to restore smgrdounlinkfork() that was removed in the older release. That function includes dropping shared buffers, which can clean up the shared buffers that may be cached by this transaction.
(10)
[RelationDropInitFork]
+ /* revert buffer-persistence changes at abort */
+ pending = (PendingRelDelete *)
+ MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
+ pending->relnode = rnode;
+ pending->op = PDOP_SET_PERSISTENCE;
+ pending->bufpersistence = false;
+ pending->backend = InvalidBackendId;
+ pending->atCommit = true;
+ pending->nestLevel = GetCurrentTransactionNestLevel();
+ pending->next = pendingDeletes;
+ pendingDeletes = pending;
+}
bufpersistence should be true.
(11)
+ BlockNumber block = 0;
...
+ DropRelFileNodeBuffers(rbnode, &pending->unlink_forknum, 1,
+ &block);
"block" is unnecessary and 0 can be passed directly.
(12)
- && pending->backend == InvalidBackendId)
+ && pending->backend == InvalidBackendId &&
+ pending->op == PDOP_DELETE)
nrels++;
It's better to put && at the beginning of the line to follow the existing code here.
(13)
+ table_close(rel, lockmode);
lockmode should be NoLock to retain the lock until transaction completion.
(14)
+ ctl.keysize = sizeof(unlogged_relation_entry);
+ ctl.entrysize = sizeof(unlogged_relation_entry);
+ hash = hash_create("unlogged hash", 32, &ctl, HASH_ELEM);
...
+ memset(key.oid, 0, sizeof(key.oid));
+ memcpy(key.oid, de->d_name, oidchars);
+ ent = hash_search(hash, &key, HASH_FIND, NULL);
keysize should be the oid member of the struct.
Regards
Takayuki Tsunakawa
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2020-12-24 08:02 Kyotaro Horiguchi <[email protected]>
parent: [email protected] <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2020-12-24 08:02 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Thanks for the comment! Sorry for the late reply.
At Fri, 4 Dec 2020 07:49:22 +0000, "[email protected]" <[email protected]> wrote in
> From: Kyotaro Horiguchi <[email protected]>
> > > No, not really. The issue is more around what happens if we crash
> > > part way through. At crash recovery time, the system catalogs are not
> > > available, because the database isn't consistent yet and, anyway, the
> > > startup process can't be bound to a database, let alone every database
> > > that might contain unlogged tables. So the sentinel that's used to
> > > decide whether to flush the contents of a table or index is the
> > > presence or absence of an _init fork, which the startup process
> > > obviously can see just fine. The _init fork also tells us what to
> > > stick in the relation when we reset it; for a table, we can just reset
> > > to an empty file, but that's not legal for indexes, so the _init fork
> > > contains a pre-initialized empty index that we can just copy over.
> > >
> > > Now, to make an unlogged table logged, you've got to at some stage
> > > remove those _init forks. But this is not a transactional operation.
> > > If you remove the _init forks and then the transaction rolls back,
> > > you've left the system an inconsistent state. If you postpone the
> > > removal until commit time, then you have a problem if it fails,
> >
> > It's true. That are the cause of headache.
> ...
> > The current implement is simple. It's enough to just discard old or
> > new relfilenode according to the current transaction ends with commit
> > or abort. Tweaking of relfilenode under use leads-in some skews in
> > some places. I used pendingDelete mechanism a bit complexified way
> > and a violated an abstraction (I think, calling AM-routines from
> > storage.c is not good.) and even introduce a new fork kind only to
> > mark a init fork as "not committed yet". There might be better way,
> > but I haven't find it.
>
> I have no alternative idea yet, too. I agree that we want to avoid them, especially introducing inittmp fork... Anyway, below are the rest of my review comments for 0001. I want to review 0002 when we have decided to go with 0001.
>
>
> (2)
> XLOG_SMGR_UNLINK seems to necessitate modification of the following comments:
>
> [src/include/catalog/storage_xlog.h]
> /*
> * Declarations for smgr-related XLOG records
> *
> * Note: we log file creation and truncation here, but logging of deletion
> * actions is handled by xact.c, because it is part of transaction commit.
> */
Sure. Rewrote it.
> [src/backend/access/transam/README]
> 3. Deleting a table, which requires an unlink() that could fail.
>
> Our approach here is to WAL-log the operation first, but to treat failure
> of the actual unlink() call as a warning rather than error condition.
> Again, this can leave an orphan file behind, but that's cheap compared to
> the alternatives. Since we can't actually do the unlink() until after
> we've committed the DROP TABLE transaction, throwing an error would be out
> of the question anyway. (It may be worth noting that the WAL entry about
> the file deletion is actually part of the commit record for the dropping
> transaction.)
Mmm. I didn't touched theDROP TABLE (RelationDropStorage) path, but I
added a brief description about INITTMP fork to the file.
====
The INITTMP fork file
--------------------------------
An INITTMP fork is created when new relation file is created to mark
the relfilenode needs to be cleaned up at recovery time. The file is
removed at transaction end but is left when the process crashes before
the transaction ends. In contrast to 4 above, failure to remove an
INITTMP file will lead to data loss, in which case the server will
shut down.
====
> (3)
> +/* This is bit-map, not ordianal numbers */
>
> There seems to be no comments using "bit-map". "Flags for ..." can be seen here and there.
I revmoed the comment and use (1 << n) notation to show the fact
instead.
> (4)
> Some wrong spellings:
>
> swithing -> switching
> alredy -> already
> recovery -> recover
> situaton -> situation
Oops! Fixed them.
> (5)
> + table_close(classRel, NoLock);
> +
> +
> +
> +
> }
>
> These empty lines can be deleted.
s/can/should/ :p. Fixed.
>
> (6)
> +/*
> + * Perform XLogInsert of an XLOG_SMGR_UNLINK record to WAL.
> + */
> +void
> +log_smgrbufpersistence(const RelFileNode *rnode, bool persistence)
> ...
> + * Make an XLOG entry reporting the file unlink.
>
> Not unlink but buffer persistence?
Silly copy-pasto. Fixed.
> (7)
> + /*
> + * index-init fork needs further initialization. ambuildempty shoud do
> + * WAL-log and file sync by itself but otherwise we do that by myself.
> + */
> + if (rel->rd_rel->relkind == RELKIND_INDEX)
> + rel->rd_indam->ambuildempty(rel);
> + else
> + {
> + log_smgrcreate(&rnode, INIT_FORKNUM);
> + smgrimmedsync(srel, INIT_FORKNUM);
> + }
> +
> + /*
> + * We have created the init fork. If server crashes before the current
> + * transaction ends the init fork left alone corrupts data while recovery.
> + * The inittmp fork works as the sentinel to identify that situaton.
> + */
> + smgrcreate(srel, INITTMP_FORKNUM, false);
> + log_smgrcreate(&rnode, INITTMP_FORKNUM);
> + smgrimmedsync(srel, INITTMP_FORKNUM);
>
> If the server crashes between these two processings, only the init fork exists. Is it correct to create the inittmp fork first?
Right. I change it that way, and did the same with the new code added
to RelationCreateStorage.
> (8)
> + if (inxact_created)
> + {
> + SMgrRelation srel = smgropen(rnode, InvalidBackendId);
> + smgrclose(srel);
> + log_smgrunlink(&rnode, INIT_FORKNUM);
> + smgrunlink(srel, INIT_FORKNUM, false);
> + log_smgrunlink(&rnode, INITTMP_FORKNUM);
> + smgrunlink(srel, INITTMP_FORKNUM, false);
> + return;
> + }
>
> smgrclose() should be called just before return.
> Isn't it necessary here to revert buffer persistence state change?
Mmm. it's a thinko. I was confused with the case of
close/unlink. Fixed all instacnes of the same.
> (9)
> +void
> +smgrunlink(SMgrRelation reln, ForkNumber forknum, bool isRedo)
> +{
> + smgrsw[reln->smgr_which].smgr_unlink(reln->smgr_rnode, forknum, isRedo);
> +}
>
> Maybe it's better to restore smgrdounlinkfork() that was removed in the older release. That function includes dropping shared buffers, which can clean up the shared buffers that may be cached by this transaction.
INITFORK/INITTMP forks cannot be loaded to shared buffer so it's no
use to drop buffers. I added a comment like that.
| /*
| * INIT/INITTMP forks never be loaded to shared buffer so no point in
| * dropping buffers for these files.
| */
| log_smgrunlink(&rnode, INIT_FORKNUM);
I removed DropRelFileNodeBuffers from PDOP_UNLINK_FORK branch in
smgrDoPendingDeletes and added an assertion and a comment instead.
| /* other forks needs to drop buffers */
| Assert(pending->unlink_forknum == INIT_FORKNUM ||
| pending->unlink_forknum == INITTMP_FORKNUM);
|
| log_smgrunlink(&pending->relnode, pending->unlink_forknum);
| smgrunlink(srel, pending->unlink_forknum, false);
> (10)
> [RelationDropInitFork]
> + /* revert buffer-persistence changes at abort */
> + pending = (PendingRelDelete *)
> + MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
> + pending->relnode = rnode;
> + pending->op = PDOP_SET_PERSISTENCE;
> + pending->bufpersistence = false;
> + pending->backend = InvalidBackendId;
> + pending->atCommit = true;
> + pending->nestLevel = GetCurrentTransactionNestLevel();
> + pending->next = pendingDeletes;
> + pendingDeletes = pending;
> +}
>
> bufpersistence should be true.
RelationDropInitFork() chnages the relation persisitence to
"persistent" so it shoud be reverted to "non-persistent (= false)" at
abort. (I agree that the function name is somewhat confusing...)
> (11)
> + BlockNumber block = 0;
> ...
> + DropRelFileNodeBuffers(rbnode, &pending->unlink_forknum, 1,
> + &block);
>
> "block" is unnecessary and 0 can be passed directly.
I removed the entire function call.
But, I don't think you're right here.
| DropRelFileNodeBuffers(RelFileNodeBackend rnode, ForkNumber *forkNum,
| int nforks, BlockNumber *firstDelBlock)
Doesn't just passing 0 lead to SEGV?
> (12)
> - && pending->backend == InvalidBackendId)
> + && pending->backend == InvalidBackendId &&
> + pending->op == PDOP_DELETE)
> nrels++;
>
> It's better to put && at the beginning of the line to follow the existing code here.
It's terrible.. Fixed.
> (13)
> + table_close(rel, lockmode);
>
> lockmode should be NoLock to retain the lock until transaction completion.
I tried to recall the reason for that, but didn't come up with
anything. Fixed.
> (14)
> + ctl.keysize = sizeof(unlogged_relation_entry);
> + ctl.entrysize = sizeof(unlogged_relation_entry);
> + hash = hash_create("unlogged hash", 32, &ctl, HASH_ELEM);
> ...
> + memset(key.oid, 0, sizeof(key.oid));
> + memcpy(key.oid, de->d_name, oidchars);
> + ent = hash_search(hash, &key, HASH_FIND, NULL);
>
> keysize should be the oid member of the struct.
It's not a problem since the first member is the oid and perhaps it
seems that I thougth to do someting more on that. Now that I don't
recall what is it and in the first place the key should be just Oid in
the context above. Fixed.
The patch is attached to the next message.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2020-12-25 00:12 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2020-12-25 00:12 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Hello.
At Thu, 24 Dec 2020 17:02:20 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> The patch is attached to the next message.
The reason for separating this message is that I modified this so that
it could solve another issue.
There's a complain about orphan files after crash. [1]
1: https://www.postgresql.org/message-id/[email protected]
That is, the case where a relation file is left alone after a server
crash that happened before the end of the transaction that has created
a relation. As I read this, I noticed this feature can solve the
issue with a small change.
This version gets changes in RelationCreateStorage and
smgrDoPendingDeletes.
Previously inittmp fork is created only along with an init fork. This
version creates one always when a relation storage file is created. As
the result ResetUnloggedRelationsInDbspaceDir removes all forks if the
inttmp fork of a logged relations is found. Now that pendingDeletes
can contain multiple entries for the same relation, it has been
modified not to close the same smgr multiple times.
- It might be better to split 0001 into two peaces.
- The function name ResetUnloggedRelationsInDbspaceDir is no longer
represents the function correctly.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2021-01-08 05:47 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2021-01-08 05:47 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Fri, 25 Dec 2020 09:12:52 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> Hello.
>
> At Thu, 24 Dec 2020 17:02:20 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> > The patch is attached to the next message.
>
> The reason for separating this message is that I modified this so that
> it could solve another issue.
>
> There's a complain about orphan files after crash. [1]
>
> 1: https://www.postgresql.org/message-id/[email protected]
>
> That is, the case where a relation file is left alone after a server
> crash that happened before the end of the transaction that has created
> a relation. As I read this, I noticed this feature can solve the
> issue with a small change.
>
> This version gets changes in RelationCreateStorage and
> smgrDoPendingDeletes.
>
> Previously inittmp fork is created only along with an init fork. This
> version creates one always when a relation storage file is created. As
> the result ResetUnloggedRelationsInDbspaceDir removes all forks if the
> inttmp fork of a logged relations is found. Now that pendingDeletes
> can contain multiple entries for the same relation, it has been
> modified not to close the same smgr multiple times.
>
> - It might be better to split 0001 into two peaces.
>
> - The function name ResetUnloggedRelationsInDbspaceDir is no longer
> represents the function correctly.
As pointed by Robert in another thread [1], persisntence of (at least)
GiST index cannot be flipped in-place due to incompatibility of fake
LSNs with real ones.
This version RelationChangePersistence() is changed not to choose
in-place method for indexes other than btree. It seems to be usable
with all kind of indexes other than Gist, but at the mement it applies
only to btrees.
1: https://www.postgresql.org/message-id/[email protected]...
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2021-01-08 08:52 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2021-01-08 08:52 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Fri, 08 Jan 2021 14:47:05 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> This version RelationChangePersistence() is changed not to choose
> in-place method for indexes other than btree. It seems to be usable
> with all kind of indexes other than Gist, but at the mement it applies
> only to btrees.
>
> 1: https://www.postgresql.org/message-id/[email protected]...
Hmm. This is not wroking correctly. I'll repost after fixint that.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2021-01-12 09:58 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2021-01-12 09:58 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Fri, 08 Jan 2021 17:52:21 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> At Fri, 08 Jan 2021 14:47:05 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> > This version RelationChangePersistence() is changed not to choose
> > in-place method for indexes other than btree. It seems to be usable
> > with all kind of indexes other than Gist, but at the mement it applies
> > only to btrees.
> >
> > 1: https://www.postgresql.org/message-id/[email protected]...
>
> Hmm. This is not wroking correctly. I'll repost after fixint that.
I think I fixed the misbehavior. ResetUnloggedRelationsInDbspaceDir()
handles file operations in the wrong order and with the wrong logic.
It also needed to drop buffers and forget fsync requests.
I thought that the two cases that this patch is expected to fix
(orphan relation files and uncommited init files) can share the same
"cleanup" fork but that is wrong. I had to add one more additional
fork to differentiate the cases of SET UNLOGGED and of creation of
UNLOGGED tables...
The attached is a new version, that seems working correctly but looks
somewhat messy. I'll continue working.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2021-01-14 08:32 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2021-01-14 08:32 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Tue, 12 Jan 2021 18:58:08 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> At Fri, 08 Jan 2021 17:52:21 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> > At Fri, 08 Jan 2021 14:47:05 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> > > This version RelationChangePersistence() is changed not to choose
> > > in-place method for indexes other than btree. It seems to be usable
> > > with all kind of indexes other than Gist, but at the mement it applies
> > > only to btrees.
> > >
> > > 1: https://www.postgresql.org/message-id/[email protected]...
> >
> > Hmm. This is not wroking correctly. I'll repost after fixint that.
>
> I think I fixed the misbehavior. ResetUnloggedRelationsInDbspaceDir()
> handles file operations in the wrong order and with the wrong logic.
> It also needed to drop buffers and forget fsync requests.
>
> I thought that the two cases that this patch is expected to fix
> (orphan relation files and uncommited init files) can share the same
> "cleanup" fork but that is wrong. I had to add one more additional
> fork to differentiate the cases of SET UNLOGGED and of creation of
> UNLOGGED tables...
>
> The attached is a new version, that seems working correctly but looks
> somewhat messy. I'll continue working.
Commit bea449c635 conflicts with this on the change of the definition
of DropRelFileNodeBuffers. The change simplified this patch by a bit:p
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2021-03-25 05:08 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2021-03-25 05:08 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
(I'm not sure when the subject was broken..)
At Thu, 14 Jan 2021 17:32:17 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> Commit bea449c635 conflicts with this on the change of the definition
> of DropRelFileNodeBuffers. The change simplified this patch by a bit:p
In this version, I got rid of the "CLEANUP FORK"s, and added a new
system "Smgr marks". The mark files have the name of the
corresponding fork file followed by ".u" (which means Uncommitted.).
"Uncommited"-marked main fork means the same as the CLEANUP2_FORKNUM
and uncommitted-marked init fork means the same as the CLEANUP_FORKNUM
in the previous version.x
I noticed that the previous version of the patch still leaves an
orphan main fork file after "BEGIN; CREATE TABLE x; ROLLBACK; (crash
before checkpoint)" since the "mark" file (or CLEANUP2_FORKNUM) is
revmoed at rollback. In this version the responsibility to remove the
mark files is moved to SyncPostCheckpoint, where main fork files are
actually removed.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2021-03-25 05:15 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2021-03-25 05:15 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Thu, 25 Mar 2021 14:08:05 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> (I'm not sure when the subject was broken..)
>
> At Thu, 14 Jan 2021 17:32:17 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> > Commit bea449c635 conflicts with this on the change of the definition
> > of DropRelFileNodeBuffers. The change simplified this patch by a bit:p
>
> In this version, I got rid of the "CLEANUP FORK"s, and added a new
> system "Smgr marks". The mark files have the name of the
> corresponding fork file followed by ".u" (which means Uncommitted.).
> "Uncommited"-marked main fork means the same as the CLEANUP2_FORKNUM
> and uncommitted-marked init fork means the same as the CLEANUP_FORKNUM
> in the previous version.x
>
> I noticed that the previous version of the patch still leaves an
> orphan main fork file after "BEGIN; CREATE TABLE x; ROLLBACK; (crash
> before checkpoint)" since the "mark" file (or CLEANUP2_FORKNUM) is
> revmoed at rollback. In this version the responsibility to remove the
> mark files is moved to SyncPostCheckpoint, where main fork files are
> actually removed.
For the record, I noticed that basebackup could be confused by the
mark files but I haven't looked that yet.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* RE: In-placre persistance change of a relation
@ 2021-12-17 09:10 Jakub Wartak <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 2 replies; 52+ messages in thread
From: Jakub Wartak @ 2021-12-17 09:10 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; [email protected] <[email protected]>; +Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>
> Kyotaro wrote:
> > In this version, I got rid of the "CLEANUP FORK"s, and added a new
> > system "Smgr marks". The mark files have the name of the
> > corresponding fork file followed by ".u" (which means Uncommitted.).
> > "Uncommited"-marked main fork means the same as the
> CLEANUP2_FORKNUM
> > and uncommitted-marked init fork means the same as the
> CLEANUP_FORKNUM
> > in the previous version.x
> >
> > I noticed that the previous version of the patch still leaves an
> > orphan main fork file after "BEGIN; CREATE TABLE x; ROLLBACK; (crash
> > before checkpoint)" since the "mark" file (or CLEANUP2_FORKNUM) is
> > revmoed at rollback. In this version the responsibility to remove the
> > mark files is moved to SyncPostCheckpoint, where main fork files are
> > actually removed.
>
> For the record, I noticed that basebackup could be confused by the mark files
> but I haven't looked that yet.
>
Good morning Kyotaro,
the patch didn't apply clean (it's from March; some hunks were failing), so I've fixed it and the combined git format-patch is attached. It did conflict with the following:
b0483263dda - Add support for SET ACCESS METHOD in ALTER TABLE
7b565843a94 - Add call to object access hook at the end of table rewrite in ALTER TABLE
9ce346eabf3 - Report progress of startup operations that take a long time.
f10f0ae420 - Replace RelationOpenSmgr() with RelationGetSmgr().
I'm especially worried if I didn't screw up something/forgot something related to the last one (rd->rd_smgr changes), but I'm getting "All 210 tests passed".
Basic demonstration of this patch (with wal_level=minimal):
create unlogged table t6 (id bigint, t text);
-- produces 110GB table, takes ~5mins
insert into t6 select nextval('s1'), repeat('A', 1000) from generate_series(1, 100000000);
alter table t6 set logged;
on baseline SET LOGGED takes: ~7min10s
on patched SET LOGGED takes: 25s
So basically one can - thanks to this patch - use his application (performing classic INSERTs/UPDATEs/DELETEs, so without the need to rewrite to use COPY) and perform literally batch upload and then just switch the tables to LOGGED.
Some more intensive testing also looks good, assuming table prepared to put pressure on WAL:
create unlogged table t_unlogged (id bigint, t text) partition by hash (id);
create unlogged table t_unlogged_h0 partition of t_unlogged FOR VALUES WITH (modulus 4, remainder 0);
[..]
create unlogged table t_unlogged_h3 partition of t_unlogged FOR VALUES WITH (modulus 4, remainder 3);
Workload would still be pretty heavy on LWLock/BufferContent,WALInsert and Lock/extend .
t_logged.sql = insert into t_logged select nextval('s1'), repeat('A', 1000) from generate_series(1, 1000); # according to pg_wal_stats.wal_bytes generates ~1MB of WAL
t_unlogged.sql = insert into t_unlogged select nextval('s1'), repeat('A', 1000) from generate_series(1, 1000); # according to pg_wal_stats.wal_bytes generates ~3kB of WAL
so using: pgbench -f <tabletypetest>.sql -T 30 -P 1 -c 32 -j 3 t
with synchronous_commit =ON(default):
with t_logged.sql: tps = 229 (lat avg = 138ms)
with t_unlogged.sql tps = 283 (lat avg = 112ms) # almost all on LWLock/WALWrite
with synchronous_commit =OFF:
with t_logged.sql: tps = 413 (lat avg = 77ms)
with t_unloged.sql: tps = 782 (lat avg = 40ms)
Afterwards switching the unlogged ~16GB partitions takes 5s per partition.
As the thread didn't get a lot of traction, I've registered it into current commitfest https://commitfest.postgresql.org/36/3461/ with You as the author and in 'Ready for review' state.
I think it behaves as almost finished one and apparently after reading all those discussions that go back over 10years+ time span about this feature, and lot of failed effort towards wal_level=noWAL I think it would be nice to finally start getting some of that of it into the core.
-Jakub Wartak.
Attachments:
[application/octet-stream] v7-0001-In-place-table-persistence-change-with-new-comman.patch (84.3K, ../../AM8PR07MB8248217A3AFEC0DD4D59ED83F6789@AM8PR07MB8248.eurprd07.prod.outlook.com/2-v7-0001-In-place-table-persistence-change-with-new-comman.patch)
download | inline diff:
From 82ea53c317b5c785d7ee91bcdaea43e9ad2c8f77 Mon Sep 17 00:00:00 2001
From: Jakub Wartak <[email protected]>
Date: Thu, 16 Dec 2021 12:03:42 +0000
Subject: [PATCH v7] In-place table persistence change with new command ALTER
TABLE ALL IN TABLESPACE SET LOGGED/UNLOGGED
Even though ALTER TABLE SET LOGGED/UNLOGGED does not require data
rewriting, currently it runs heap rewrite which causes large amount of
file I/O. This patch makes the command run without heap rewrite.
Addition to that, SET LOGGED while wal_level > minimal emits WAL using
XLOG_FPI instead of massive number of HEAP_INSERT's, which should be
smaller.
Also this allows for the cleanup of files left behind in the crash of
the transaction that created it.
ALTER TABLE ALL IN TABLESPACE SET LOGGED/UNLOGGED: To ease invoking
ALTER TABLE SET LOGGED/UNLOGGED, this command changes relation persistence
of all tables in the specified tablespace.
---
src/backend/access/rmgrdesc/smgrdesc.c | 52 ++++
src/backend/access/transam/README | 8 +
src/backend/access/transam/xlog.c | 17 ++
src/backend/catalog/storage.c | 518 +++++++++++++++++++++++++++++++--
src/backend/commands/tablecmds.c | 374 ++++++++++++++++++++++--
src/backend/nodes/copyfuncs.c | 16 +
src/backend/nodes/equalfuncs.c | 15 +
src/backend/parser/gram.y | 20 ++
src/backend/replication/basebackup.c | 3 +-
src/backend/storage/buffer/bufmgr.c | 88 ++++++
src/backend/storage/file/fd.c | 4 +-
src/backend/storage/file/reinit.c | 318 ++++++++++++++------
src/backend/storage/smgr/md.c | 92 +++++-
src/backend/storage/smgr/smgr.c | 32 ++
src/backend/storage/sync/sync.c | 20 +-
src/backend/tcop/utility.c | 11 +
src/bin/pg_rewind/parsexlog.c | 24 ++
src/common/relpath.c | 47 +--
src/include/catalog/storage.h | 2 +
src/include/catalog/storage_xlog.h | 42 ++-
src/include/commands/tablecmds.h | 2 +
src/include/common/relpath.h | 9 +-
src/include/nodes/nodes.h | 1 +
src/include/nodes/parsenodes.h | 9 +
src/include/storage/bufmgr.h | 2 +
src/include/storage/fd.h | 1 +
src/include/storage/md.h | 8 +-
src/include/storage/reinit.h | 10 +-
src/include/storage/smgr.h | 17 ++
29 files changed, 1583 insertions(+), 179 deletions(-)
diff --git a/src/backend/access/rmgrdesc/smgrdesc.c b/src/backend/access/rmgrdesc/smgrdesc.c
index 7755553d57..d251f22207 100644
--- a/src/backend/access/rmgrdesc/smgrdesc.c
+++ b/src/backend/access/rmgrdesc/smgrdesc.c
@@ -40,6 +40,49 @@ smgr_desc(StringInfo buf, XLogReaderState *record)
xlrec->blkno, xlrec->flags);
pfree(path);
}
+ else if (info == XLOG_SMGR_UNLINK)
+ {
+ xl_smgr_unlink *xlrec = (xl_smgr_unlink *) rec;
+ char *path = relpathperm(xlrec->rnode, xlrec->forkNum);
+
+ appendStringInfoString(buf, path);
+ pfree(path);
+ }
+ else if (info == XLOG_SMGR_MARK)
+ {
+ xl_smgr_mark *xlrec = (xl_smgr_mark *) rec;
+ char *path = GetRelationPath(xlrec->rnode.dbNode,
+ xlrec->rnode.spcNode,
+ xlrec->rnode.relNode,
+ InvalidBackendId,
+ xlrec->forkNum, xlrec->mark);
+ char *action;
+
+ switch (xlrec->action)
+ {
+ case XLOG_SMGR_MARK_CREATE:
+ action = "CREATE";
+ break;
+ case XLOG_SMGR_MARK_UNLINK:
+ action = "DELETE";
+ break;
+ default:
+ action = "<unknown action>";
+ break;
+ }
+
+ appendStringInfo(buf, "%s %s", action, path);
+ pfree(path);
+ }
+ else if (info == XLOG_SMGR_BUFPERSISTENCE)
+ {
+ xl_smgr_bufpersistence *xlrec = (xl_smgr_bufpersistence *) rec;
+ char *path = relpathperm(xlrec->rnode, MAIN_FORKNUM);
+
+ appendStringInfoString(buf, path);
+ appendStringInfo(buf, " persistence %d", xlrec->persistence);
+ pfree(path);
+ }
}
const char *
@@ -55,6 +98,15 @@ smgr_identify(uint8 info)
case XLOG_SMGR_TRUNCATE:
id = "TRUNCATE";
break;
+ case XLOG_SMGR_UNLINK:
+ id = "UNLINK";
+ break;
+ case XLOG_SMGR_MARK:
+ id = "MARK";
+ break;
+ case XLOG_SMGR_BUFPERSISTENCE:
+ id = "BUFPERSISTENCE";
+ break;
}
return id;
diff --git a/src/backend/access/transam/README b/src/backend/access/transam/README
index 1edc8180c1..7cf77e4a02 100644
--- a/src/backend/access/transam/README
+++ b/src/backend/access/transam/README
@@ -724,6 +724,14 @@ we must panic and abort recovery. The DBA will have to manually clean up
then restart recovery. This is part of the reason for not writing a WAL
entry until we've successfully done the original action.
+The Smgr MARK files
+--------------------------------
+
+A smgr mark files is created when a new relation file is created to
+mark the relfilenode needs to be cleaned up at recovery time. In
+contrast to 4 above, failure to remove smgr mark files will lead to
+data loss, in which case the server will shut down.
+
Skipping WAL for New RelFileNode
--------------------------------
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 1e1fbe957f..59f4c2eacf 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -40,6 +40,7 @@
#include "catalog/catversion.h"
#include "catalog/pg_control.h"
#include "catalog/pg_database.h"
+#include "catalog/storage.h"
#include "commands/progress.h"
#include "commands/tablespace.h"
#include "common/controldata_utils.h"
@@ -4564,6 +4565,14 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
{
ereport(DEBUG1,
(errmsg_internal("reached end of WAL in pg_wal, entering archive recovery")));
+
+ /* cleanup garbage files left during crash recovery */
+ ResetUnloggedRelations(UNLOGGED_RELATION_DROP_BUFFER |
+ UNLOGGED_RELATION_CLEANUP);
+
+ /* run rollback cleanup if any */
+ smgrDoPendingDeletes(false);
+
InArchiveRecovery = true;
if (StandbyModeRequested)
StandbyMode = true;
@@ -7824,6 +7833,14 @@ StartupXLOG(void)
}
}
+ /* cleanup garbage files left during crash recovery */
+ if (!InArchiveRecovery)
+ ResetUnloggedRelations(UNLOGGED_RELATION_DROP_BUFFER |
+ UNLOGGED_RELATION_CLEANUP);
+
+ /* run rollback cleanup if any */
+ smgrDoPendingDeletes(false);
+
/* Allow resource managers to do any required cleanup. */
for (rmid = 0; rmid <= RM_MAX_ID; rmid++)
{
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index c5ad28d71f..a3e250515c 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -19,6 +19,7 @@
#include "postgres.h"
+#include "access/amapi.h"
#include "access/parallel.h"
#include "access/visibilitymap.h"
#include "access/xact.h"
@@ -27,6 +28,7 @@
#include "access/xlogutils.h"
#include "catalog/storage.h"
#include "catalog/storage_xlog.h"
+#include "common/hashfn.h"
#include "miscadmin.h"
#include "storage/freespace.h"
#include "storage/smgr.h"
@@ -57,9 +59,18 @@ int wal_skip_threshold = 2048; /* in kilobytes */
* but I'm being paranoid.
*/
+#define PDOP_DELETE (1 << 0)
+#define PDOP_UNLINK_FORK (1 << 1)
+#define PDOP_UNLINK_MARK (1 << 2)
+#define PDOP_SET_PERSISTENCE (1 << 3)
+
typedef struct PendingRelDelete
{
RelFileNode relnode; /* relation that may need to be deleted */
+ int op; /* operation mask */
+ bool bufpersistence; /* buffer persistence to set */
+ int unlink_forknum; /* forknum to unlink */
+ StorageMarks unlink_mark; /* mark to unlink */
BackendId backend; /* InvalidBackendId if not a temp rel */
bool atCommit; /* T=delete at commit; F=delete at abort */
int nestLevel; /* xact nesting level of request */
@@ -75,6 +86,24 @@ typedef struct PendingRelSync
static PendingRelDelete *pendingDeletes = NULL; /* head of linked list */
HTAB *pendingSyncHash = NULL;
+typedef struct SRelHashEntry
+{
+ SMgrRelation srel;
+ char status; /* for simplehash use */
+} SRelHashEntry;
+
+/* define hashtable for workarea for pending deletes */
+#define SH_PREFIX srelhash
+#define SH_ELEMENT_TYPE SRelHashEntry
+#define SH_KEY_TYPE SMgrRelation
+#define SH_KEY srel
+#define SH_HASH_KEY(tb, key) \
+ hash_bytes((unsigned char *)&key, sizeof(SMgrRelation))
+#define SH_EQUAL(tb, a, b) (memcmp(&a, &b, sizeof(SMgrRelation)) == 0)
+#define SH_SCOPE static inline
+#define SH_DEFINE
+#define SH_DECLARE
+#include "lib/simplehash.h"
/*
* AddPendingSync
@@ -143,22 +172,48 @@ RelationCreateStorage(RelFileNode rnode, char relpersistence)
return NULL; /* placate compiler */
}
+ /*
+ * We are going to create a new storage file. If server crashes before the
+ * current transaction ends the file needs to be cleaned up but there's no
+ * clue to the orphan files. The SMGR_MARK_UNCOMMITED mark file works as
+ * the signal of that situation.
+ */
srel = smgropen(rnode, backend);
+ log_smgrcreatemark(&rnode, MAIN_FORKNUM, SMGR_MARK_UNCOMMITTED);
+ smgrcreatemark(srel, MAIN_FORKNUM, SMGR_MARK_UNCOMMITTED, false);
smgrcreate(srel, MAIN_FORKNUM, false);
if (needs_wal)
log_smgrcreate(&srel->smgr_rnode.node, MAIN_FORKNUM);
- /* Add the relation to the list of stuff to delete at abort */
+ /*
+ * Add the relation to the list of stuff to delete at abort. We don't
+ * remove the mark file at commit. It needs to persiste until the main fork
+ * file is actually deleted. See SyncPostCheckpoint.
+ */
pending = (PendingRelDelete *)
MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
pending->relnode = rnode;
+ pending->op = PDOP_DELETE;
pending->backend = backend;
pending->atCommit = false; /* delete if abort */
pending->nestLevel = GetCurrentTransactionNestLevel();
pending->next = pendingDeletes;
pendingDeletes = pending;
+ /* drop cleanup fork at commit */
+ pending = (PendingRelDelete *)
+ MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
+ pending->relnode = rnode;
+ pending->op = PDOP_UNLINK_MARK;
+ pending->unlink_forknum = MAIN_FORKNUM;
+ pending->unlink_mark = SMGR_MARK_UNCOMMITTED;
+ pending->backend = backend;
+ pending->atCommit = true;
+ pending->nestLevel = GetCurrentTransactionNestLevel();
+ pending->next = pendingDeletes;
+ pendingDeletes = pending;
+
if (relpersistence == RELPERSISTENCE_PERMANENT && !XLogIsNeeded())
{
Assert(backend == InvalidBackendId);
@@ -169,6 +224,207 @@ RelationCreateStorage(RelFileNode rnode, char relpersistence)
}
/*
+ * RelationCreateInitFork
+ * Create physical storage for the init fork of a relation.
+ *
+ * Create the init fork for the relation.
+ *
+ * This function is transactional. The creation is WAL-logged, and if the
+ * transaction aborts later on, the init fork will be removed.
+ */
+void
+RelationCreateInitFork(Relation rel)
+{
+ RelFileNode rnode = rel->rd_node;
+ PendingRelDelete *pending;
+ SMgrRelation srel;
+ PendingRelDelete *prev;
+ PendingRelDelete *next;
+ bool create = true;
+
+ /* switch buffer persistence */
+ SetRelationBuffersPersistence(RelationGetSmgr(rel), false, false);
+
+ /*
+ * If we have entries for init-fork operation of this relation, that means
+ * that we have already registered pending delete entries to drop
+ * preexisting init fork since before the current transaction started. This
+ * function reverts that change just by removing the entries.
+ */
+ prev = NULL;
+ for (pending = pendingDeletes; pending != NULL; pending = next)
+ {
+ next = pending->next;
+ if (RelFileNodeEquals(rnode, pending->relnode) &&
+ (pending->op & PDOP_DELETE) == 0 &&
+ (pending->unlink_forknum == INIT_FORKNUM ||
+ (pending->op & PDOP_SET_PERSISTENCE) != 0))
+ {
+ if (prev)
+ prev->next = next;
+ else
+ pendingDeletes = next;
+ pfree(pending);
+
+ create = false;
+ }
+ else
+ {
+ /* unrelated entry, don't touch it */
+ prev = pending;
+ }
+ }
+
+ if (!create)
+ return;
+
+ /*
+ * We are going to create the init fork. If server crashes before the
+ * current transaction ends the init fork left alone corrupts data while
+ * recovery. The cleanup fork works as the sentinel to identify that
+ * situation.
+ */
+ srel = smgropen(rnode, InvalidBackendId);
+ log_smgrcreatemark(&rnode, INIT_FORKNUM, SMGR_MARK_UNCOMMITTED);
+ smgrcreatemark(srel, INIT_FORKNUM, SMGR_MARK_UNCOMMITTED, false);
+
+ /* We don't have existing init fork, create it. */
+ smgrcreate(srel, INIT_FORKNUM, false);
+
+ /*
+ * index-init fork needs further initialization. ambuildempty shoud do
+ * WAL-log and file sync by itself but otherwise we do that by ourselves.
+ */
+ if (rel->rd_rel->relkind == RELKIND_INDEX)
+ rel->rd_indam->ambuildempty(rel);
+ else
+ {
+ log_smgrcreate(&rnode, INIT_FORKNUM);
+ smgrimmedsync(srel, INIT_FORKNUM);
+ }
+
+ /* drop the init fork, mark file and revert persistence at abort */
+ pending = (PendingRelDelete *)
+ MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
+ pending->relnode = rnode;
+ pending->op = PDOP_UNLINK_FORK | PDOP_UNLINK_MARK | PDOP_SET_PERSISTENCE;
+ pending->unlink_forknum = INIT_FORKNUM;
+ pending->unlink_mark = SMGR_MARK_UNCOMMITTED;
+ pending->bufpersistence = true;
+ pending->backend = InvalidBackendId;
+ pending->atCommit = false;
+ pending->nestLevel = GetCurrentTransactionNestLevel();
+ pending->next = pendingDeletes;
+ pendingDeletes = pending;
+
+ /* drop mark file at commit */
+ pending = (PendingRelDelete *)
+ MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
+ pending->relnode = rnode;
+ pending->op = PDOP_UNLINK_MARK;
+ pending->unlink_forknum = INIT_FORKNUM;
+ pending->unlink_mark = SMGR_MARK_UNCOMMITTED;
+ pending->backend = InvalidBackendId;
+ pending->atCommit = true;
+ pending->nestLevel = GetCurrentTransactionNestLevel();
+ pending->next = pendingDeletes;
+ pendingDeletes = pending;
+}
+
+/*
+ * RelationDropInitFork
+ * Delete physical storage for the init fork of a relation.
+ *
+ * Register pending-delete of the init fork. The real deletion is performed by
+ * smgrDoPendingDeletes at commit.
+ *
+ * This function is transactional. If the transaction aborts later on, the
+ * deletion doesn't happen.
+ */
+void
+RelationDropInitFork(Relation rel)
+{
+ RelFileNode rnode = rel->rd_node;
+ PendingRelDelete *pending;
+ PendingRelDelete *prev;
+ PendingRelDelete *next;
+ bool inxact_created = false;
+
+ /* switch buffer persistence */
+ SetRelationBuffersPersistence(RelationGetSmgr(rel), true, false);
+
+ /*
+ * If we have entries for init-fork operations of this relation, that means
+ * that we have created the init fork in the current transaction. We
+ * remove the init fork and mark file immediately in that case. Otherwise
+ * just reister pending-delete for the existing init fork.
+ */
+ prev = NULL;
+ for (pending = pendingDeletes; pending != NULL; pending = next)
+ {
+ next = pending->next;
+ if (RelFileNodeEquals(rnode, pending->relnode) &&
+ (pending->op & PDOP_DELETE) == 0 &&
+ (pending->unlink_forknum == INIT_FORKNUM ||
+ (pending->op & PDOP_SET_PERSISTENCE) != 0))
+ {
+ /* unlink list entry */
+ if (prev)
+ prev->next = next;
+ else
+ pendingDeletes = next;
+ pfree(pending);
+
+ inxact_created = true;
+ }
+ else
+ {
+ /* unrelated entry, don't touch it */
+ prev = pending;
+ }
+ }
+
+ if (inxact_created)
+ {
+ SMgrRelation srel = smgropen(rnode, InvalidBackendId);
+
+ /*
+ * INIT forks never be loaded to shared buffer so no point in dropping
+ * buffers for such files.
+ */
+ log_smgrunlinkmark(&rnode, INIT_FORKNUM, SMGR_MARK_UNCOMMITTED);
+ smgrunlinkmark(srel, INIT_FORKNUM, SMGR_MARK_UNCOMMITTED, false);
+ log_smgrunlink(&rnode, INIT_FORKNUM);
+ smgrunlink(srel, INIT_FORKNUM, false);
+ return;
+ }
+
+ /* register drop of this init fork file at commit */
+ pending = (PendingRelDelete *)
+ MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
+ pending->relnode = rnode;
+ pending->op = PDOP_UNLINK_FORK;
+ pending->unlink_forknum = INIT_FORKNUM;
+ pending->backend = InvalidBackendId;
+ pending->atCommit = true;
+ pending->nestLevel = GetCurrentTransactionNestLevel();
+ pending->next = pendingDeletes;
+ pendingDeletes = pending;
+
+ /* revert buffer-persistence changes at abort */
+ pending = (PendingRelDelete *)
+ MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
+ pending->relnode = rnode;
+ pending->op = PDOP_SET_PERSISTENCE;
+ pending->bufpersistence = false;
+ pending->backend = InvalidBackendId;
+ pending->atCommit = false;
+ pending->nestLevel = GetCurrentTransactionNestLevel();
+ pending->next = pendingDeletes;
+ pendingDeletes = pending;
+}
+
+/*
* Perform XLogInsert of an XLOG_SMGR_CREATE record to WAL.
*/
void
@@ -188,6 +444,88 @@ log_smgrcreate(const RelFileNode *rnode, ForkNumber forkNum)
}
/*
+ * Perform XLogInsert of an XLOG_SMGR_UNLINK record to WAL.
+ */
+void
+log_smgrunlink(const RelFileNode *rnode, ForkNumber forkNum)
+{
+ xl_smgr_unlink xlrec;
+
+ /*
+ * Make an XLOG entry reporting the file unlink.
+ */
+ xlrec.rnode = *rnode;
+ xlrec.forkNum = forkNum;
+
+ XLogBeginInsert();
+ XLogRegisterData((char *) &xlrec, sizeof(xlrec));
+ XLogInsert(RM_SMGR_ID, XLOG_SMGR_UNLINK | XLR_SPECIAL_REL_UPDATE);
+}
+
+/*
+ * Perform XLogInsert of an XLOG_SMGR_CREATEMARK record to WAL.
+ */
+void
+log_smgrcreatemark(const RelFileNode *rnode, ForkNumber forkNum,
+ StorageMarks mark)
+{
+ xl_smgr_mark xlrec;
+
+ /*
+ * Make an XLOG entry reporting the file creation.
+ */
+ xlrec.rnode = *rnode;
+ xlrec.forkNum = forkNum;
+ xlrec.mark = mark;
+ xlrec.action = XLOG_SMGR_MARK_CREATE;
+
+ XLogBeginInsert();
+ XLogRegisterData((char *) &xlrec, sizeof(xlrec));
+ XLogInsert(RM_SMGR_ID, XLOG_SMGR_MARK | XLR_SPECIAL_REL_UPDATE);
+}
+
+/*
+ * Perform XLogInsert of an XLOG_SMGR_UNLINKMARK record to WAL.
+ */
+void
+log_smgrunlinkmark(const RelFileNode *rnode, ForkNumber forkNum,
+ StorageMarks mark)
+{
+ xl_smgr_mark xlrec;
+
+ /*
+ * Make an XLOG entry reporting the file creation.
+ */
+ xlrec.rnode = *rnode;
+ xlrec.forkNum = forkNum;
+ xlrec.mark = mark;
+ xlrec.action = XLOG_SMGR_MARK_UNLINK;
+
+ XLogBeginInsert();
+ XLogRegisterData((char *) &xlrec, sizeof(xlrec));
+ XLogInsert(RM_SMGR_ID, XLOG_SMGR_MARK | XLR_SPECIAL_REL_UPDATE);
+}
+
+/*
+ * Perform XLogInsert of an XLOG_SMGR_BUFPERSISTENCE record to WAL.
+ */
+void
+log_smgrbufpersistence(const RelFileNode *rnode, bool persistence)
+{
+ xl_smgr_bufpersistence xlrec;
+
+ /*
+ * Make an XLOG entry reporting the change of buffer persistence.
+ */
+ xlrec.rnode = *rnode;
+ xlrec.persistence = persistence;
+
+ XLogBeginInsert();
+ XLogRegisterData((char *) &xlrec, sizeof(xlrec));
+ XLogInsert(RM_SMGR_ID, XLOG_SMGR_BUFPERSISTENCE | XLR_SPECIAL_REL_UPDATE);
+}
+
+/*
* RelationDropStorage
* Schedule unlinking of physical storage at transaction commit.
*/
@@ -200,6 +538,7 @@ RelationDropStorage(Relation rel)
pending = (PendingRelDelete *)
MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
pending->relnode = rel->rd_node;
+ pending->op = PDOP_DELETE;
pending->backend = rel->rd_backend;
pending->atCommit = true; /* delete if commit */
pending->nestLevel = GetCurrentTransactionNestLevel();
@@ -618,59 +957,104 @@ smgrDoPendingDeletes(bool isCommit)
int nrels = 0,
maxrels = 0;
SMgrRelation *srels = NULL;
+ srelhash_hash *close_srels = NULL;
+ bool found;
prev = NULL;
for (pending = pendingDeletes; pending != NULL; pending = next)
{
+ SMgrRelation srel;
+
next = pending->next;
if (pending->nestLevel < nestLevel)
{
/* outer-level entries should not be processed yet */
prev = pending;
+ continue;
}
+
+ /* unlink list entry first, so we don't retry on failure */
+ if (prev)
+ prev->next = next;
else
+ pendingDeletes = next;
+
+ if (pending->atCommit != isCommit)
{
- /* unlink list entry first, so we don't retry on failure */
- if (prev)
- prev->next = next;
- else
- pendingDeletes = next;
- /* do deletion if called for */
- if (pending->atCommit == isCommit)
- {
- SMgrRelation srel;
+ /* must explicitly free the list entry */
+ pfree(pending);
+ /* prev does not change */
+ continue;
+ }
- srel = smgropen(pending->relnode, pending->backend);
+ if (close_srels == NULL)
+ close_srels = srelhash_create(CurrentMemoryContext, 32, NULL);
- /* allocate the initial array, or extend it, if needed */
- if (maxrels == 0)
- {
- maxrels = 8;
- srels = palloc(sizeof(SMgrRelation) * maxrels);
- }
- else if (maxrels <= nrels)
- {
- maxrels *= 2;
- srels = repalloc(srels, sizeof(SMgrRelation) * maxrels);
- }
+ srel = smgropen(pending->relnode, pending->backend);
+
+ /* Uniquify the smgr relations */
+ srelhash_insert(close_srels, srel, &found);
- srels[nrels++] = srel;
+ if (pending->op & PDOP_DELETE)
+ {
+ /* allocate the initial array, or extend it, if needed */
+ if (maxrels == 0)
+ {
+ maxrels = 8;
+ srels = palloc(sizeof(SMgrRelation) * maxrels);
}
- /* must explicitly free the list entry */
- pfree(pending);
- /* prev does not change */
+ else if (maxrels <= nrels)
+ {
+ maxrels *= 2;
+ srels = repalloc(srels, sizeof(SMgrRelation) * maxrels);
+ }
+
+ srels[nrels++] = srel;
}
+
+ if (pending->op & PDOP_UNLINK_FORK)
+ {
+ /* other forks needs to drop buffers */
+ Assert(pending->unlink_forknum == INIT_FORKNUM);
+
+ /* Don't emit wal while recovery. */
+ if (!InRecovery)
+ log_smgrunlink(&pending->relnode, pending->unlink_forknum);
+ smgrunlink(srel, pending->unlink_forknum, false);
+ }
+
+ if (pending->op & PDOP_UNLINK_MARK)
+ {
+ if (!InRecovery)
+ log_smgrunlinkmark(&pending->relnode,
+ pending->unlink_forknum,
+ pending->unlink_mark);
+ smgrunlinkmark(srel, pending->unlink_forknum,
+ pending->unlink_mark, InRecovery);
+ }
+
+ if (pending->op & PDOP_SET_PERSISTENCE)
+ SetRelationBuffersPersistence(srel, pending->bufpersistence,
+ InRecovery);
}
if (nrels > 0)
{
smgrdounlinkall(srels, nrels, false);
-
- for (int i = 0; i < nrels; i++)
- smgrclose(srels[i]);
-
pfree(srels);
}
+
+ if (close_srels)
+ {
+ srelhash_iterator i;
+ SRelHashEntry *ent;
+
+ /* close smgr relatoins */
+ srelhash_start_iterate(close_srels, &i);
+ while ((ent = srelhash_iterate(close_srels, &i)) != NULL)
+ smgrclose(ent->srel);
+ srelhash_destroy(close_srels);
+ }
}
/*
@@ -840,7 +1224,8 @@ smgrGetPendingDeletes(bool forCommit, RelFileNode **ptr)
for (pending = pendingDeletes; pending != NULL; pending = pending->next)
{
if (pending->nestLevel >= nestLevel && pending->atCommit == forCommit
- && pending->backend == InvalidBackendId)
+ && pending->backend == InvalidBackendId
+ && pending->op == PDOP_DELETE)
nrels++;
}
if (nrels == 0)
@@ -853,7 +1238,8 @@ smgrGetPendingDeletes(bool forCommit, RelFileNode **ptr)
for (pending = pendingDeletes; pending != NULL; pending = pending->next)
{
if (pending->nestLevel >= nestLevel && pending->atCommit == forCommit
- && pending->backend == InvalidBackendId)
+ && pending->backend == InvalidBackendId &&
+ pending->op == PDOP_DELETE)
{
*rptr = pending->relnode;
rptr++;
@@ -933,6 +1319,15 @@ smgr_redo(XLogReaderState *record)
reln = smgropen(xlrec->rnode, InvalidBackendId);
smgrcreate(reln, xlrec->forkNum, true);
}
+ else if (info == XLOG_SMGR_UNLINK)
+ {
+ xl_smgr_unlink *xlrec = (xl_smgr_unlink *) XLogRecGetData(record);
+ SMgrRelation reln;
+
+ reln = smgropen(xlrec->rnode, InvalidBackendId);
+ smgrunlink(reln, xlrec->forkNum, true);
+ smgrclose(reln);
+ }
else if (info == XLOG_SMGR_TRUNCATE)
{
xl_smgr_truncate *xlrec = (xl_smgr_truncate *) XLogRecGetData(record);
@@ -1021,6 +1416,65 @@ smgr_redo(XLogReaderState *record)
FreeFakeRelcacheEntry(rel);
}
+ else if (info == XLOG_SMGR_MARK)
+ {
+ xl_smgr_mark *xlrec = (xl_smgr_mark *) XLogRecGetData(record);
+ SMgrRelation reln;
+ PendingRelDelete *pending;
+ bool created = false;
+
+ reln = smgropen(xlrec->rnode, InvalidBackendId);
+ switch (xlrec->action)
+ {
+ case XLOG_SMGR_MARK_CREATE:
+ smgrcreatemark(reln, xlrec->forkNum, xlrec->mark, true);
+ created = true;
+ break;
+ case XLOG_SMGR_MARK_UNLINK:
+ smgrunlinkmark(reln, xlrec->forkNum, xlrec->mark, true);
+ break;
+ default:
+ elog(ERROR, "unknown smgr_mark action \"%c\"", xlrec->mark);
+ }
+
+ if (created)
+ {
+ /* revert mark file operation at abort */
+ pending = (PendingRelDelete *)
+ MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
+ pending->relnode = xlrec->rnode;
+ pending->op = PDOP_UNLINK_MARK;
+ pending->unlink_forknum = xlrec->forkNum;
+ pending->unlink_mark = xlrec->mark;
+ pending->backend = InvalidBackendId;
+ pending->atCommit = false;
+ pending->nestLevel = GetCurrentTransactionNestLevel();
+ pending->next = pendingDeletes;
+ pendingDeletes = pending;
+ }
+ }
+ else if (info == XLOG_SMGR_BUFPERSISTENCE)
+ {
+ xl_smgr_bufpersistence *xlrec =
+ (xl_smgr_bufpersistence *) XLogRecGetData(record);
+ SMgrRelation reln;
+ PendingRelDelete *pending;
+
+ reln = smgropen(xlrec->rnode, InvalidBackendId);
+ SetRelationBuffersPersistence(reln, xlrec->persistence, true);
+
+ /* revert buffer-persistence changes at abort */
+ pending = (PendingRelDelete *)
+ MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
+ pending->relnode = xlrec->rnode;
+ pending->op = PDOP_SET_PERSISTENCE;
+ pending->bufpersistence = !xlrec->persistence;
+ pending->backend = InvalidBackendId;
+ pending->atCommit = false;
+ pending->nestLevel = GetCurrentTransactionNestLevel();
+ pending->next = pendingDeletes;
+ pendingDeletes = pending;
+ }
else
elog(PANIC, "smgr_redo: unknown op code %u", info);
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index bf42587e38..726a0484f9 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -52,6 +52,7 @@
#include "commands/defrem.h"
#include "commands/event_trigger.h"
#include "commands/policy.h"
+#include "commands/progress.h"
#include "commands/sequence.h"
#include "commands/tablecmds.h"
#include "commands/tablespace.h"
@@ -5330,6 +5331,168 @@ ATParseTransformCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
}
/*
+ * RelationChangePersistence: do in-place persistence change of a relation
+ */
+static void
+RelationChangePersistence(AlteredTableInfo *tab, char persistence,
+ LOCKMODE lockmode)
+{
+ Relation rel;
+ Relation classRel;
+ HeapTuple tuple,
+ newtuple;
+ Datum new_val[Natts_pg_class];
+ bool new_null[Natts_pg_class],
+ new_repl[Natts_pg_class];
+ int i;
+ List *relids;
+ ListCell *lc_oid;
+
+ Assert(tab->rewrite == AT_REWRITE_ALTER_PERSISTENCE);
+ Assert(lockmode == AccessExclusiveLock);
+
+ /*
+ * Under the following condition, we need to call ATRewriteTable, which
+ * cannot be false in the AT_REWRITE_ALTER_PERSISTENCE case.
+ */
+ Assert(tab->constraints == NULL && tab->partition_constraint == NULL &&
+ tab->newvals == NULL && !tab->verify_new_notnull);
+
+ rel = table_open(tab->relid, lockmode);
+
+ Assert(rel->rd_rel->relpersistence != persistence);
+
+ elog(DEBUG1, "perform im-place persistnce change");
+
+ RelationGetSmgr(rel);
+
+ /*
+ * First we collect all relations that we need to change persistence.
+ */
+
+ /* Collect OIDs of indexes and toast relations */
+ relids = RelationGetIndexList(rel);
+ relids = lcons_oid(rel->rd_id, relids);
+
+ /* Add toast relation if any */
+ if (OidIsValid(rel->rd_rel->reltoastrelid))
+ {
+ List *toastidx;
+ Relation toastrel = table_open(rel->rd_rel->reltoastrelid, lockmode);
+
+ RelationGetSmgr(toastrel);
+ relids = lappend_oid(relids, rel->rd_rel->reltoastrelid);
+ toastidx = RelationGetIndexList(toastrel);
+ relids = list_concat(relids, toastidx);
+ pfree(toastidx);
+ table_close(toastrel, NoLock);
+ }
+
+ table_close(rel, NoLock);
+
+ /* Make changes in storage */
+ classRel = table_open(RelationRelationId, RowExclusiveLock);
+
+ foreach (lc_oid, relids)
+ {
+ Oid reloid = lfirst_oid(lc_oid);
+ Relation r = relation_open(reloid, lockmode);
+
+ /*
+ * Some access methods do not accept in-place persistence change. For
+ * example, GiST uses page LSNs to figure out whether a block has
+ * changed, where UNLOGGED GiST indexes use fake LSNs that are
+ * incompatible with real LSNs used for LOGGED ones.
+ *
+ * XXXX: We don't bother allowing in-place persistence change for index
+ * methods other than btree for now.
+ */
+ if (r->rd_rel->relkind == RELKIND_INDEX &&
+ r->rd_rel->relam != BTREE_AM_OID)
+ {
+ int reindex_flags;
+
+ /* reindex doesn't allow concurrent use of the index */
+ table_close(r, NoLock);
+
+ reindex_flags =
+ REINDEX_REL_SUPPRESS_INDEX_USE |
+ REINDEX_REL_CHECK_CONSTRAINTS;
+
+ /* Set the same persistence with the parent relation. */
+ if (persistence == RELPERSISTENCE_UNLOGGED)
+ reindex_flags |= REINDEX_REL_FORCE_INDEXES_UNLOGGED;
+ else
+ reindex_flags |= REINDEX_REL_FORCE_INDEXES_PERMANENT;
+
+ reindex_index(reloid, reindex_flags, persistence, 0);
+
+ continue;
+ }
+
+ /* Create or drop init fork */
+ if (persistence == RELPERSISTENCE_UNLOGGED)
+ RelationCreateInitFork(r);
+ else
+ RelationDropInitFork(r);
+
+ /*
+ * When this relation gets WAL-logged, immediately sync all files but
+ * initfork to establish the initial state on storage. Buffers have
+ * already flushed out by RelationCreate(Drop)InitFork called just
+ * above. Initfork should have been synced as needed.
+ */
+ if (persistence == RELPERSISTENCE_PERMANENT)
+ {
+ for (i = 0 ; i < INIT_FORKNUM ; i++)
+ {
+ if (smgrexists(RelationGetSmgr(r), i))
+ smgrimmedsync(RelationGetSmgr(r), i);
+ }
+ }
+
+ /* Update catalog */
+ tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(reloid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for relation %u", reloid);
+
+ memset(new_val, 0, sizeof(new_val));
+ memset(new_null, false, sizeof(new_null));
+ memset(new_repl, false, sizeof(new_repl));
+
+ new_val[Anum_pg_class_relpersistence - 1] = CharGetDatum(persistence);
+ new_null[Anum_pg_class_relpersistence - 1] = false;
+ new_repl[Anum_pg_class_relpersistence - 1] = true;
+
+ newtuple = heap_modify_tuple(tuple, RelationGetDescr(classRel),
+ new_val, new_null, new_repl);
+
+ CatalogTupleUpdate(classRel, &newtuple->t_self, newtuple);
+ heap_freetuple(newtuple);
+
+ /*
+ * While wal_level >= replica, switching to LOGGED requires the
+ * relation content to be WAL-logged to recover the table.
+ */
+ if (persistence == RELPERSISTENCE_PERMANENT && XLogIsNeeded())
+ {
+ ForkNumber fork;
+
+ for (fork = 0; fork < INIT_FORKNUM ; fork++)
+ {
+ if (smgrexists(RelationGetSmgr(r), fork))
+ log_newpage_range(r, fork,
+ 0, smgrnblocks(RelationGetSmgr(r), fork), false);
+ }
+ }
+
+ table_close(r, NoLock);
+ }
+
+ table_close(classRel, NoLock);
+}
+
+/*
* ATRewriteTables: ALTER TABLE phase 3
*/
static void
@@ -5474,32 +5637,55 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
* persistence. That wouldn't work for pg_class, but that can't be
* unlogged anyway.
*/
- OIDNewHeap = make_new_heap(tab->relid, NewTableSpace, NewAccessMethod,
+
+ if (tab->rewrite == AT_REWRITE_ALTER_PERSISTENCE)
+ RelationChangePersistence(tab, persistence, lockmode);
+ else
+ {
+ /*
+ * Create transient table that will receive the modified data.
+ *
+ * Ensure it is marked correctly as logged or unlogged. We
+ * have to do this here so that buffers for the new relfilenode
+ * will have the right persistence set, and at the same time
+ * ensure that the original filenode's buffers will get read in
+ * with the correct setting (i.e. the original one). Otherwise
+ * a rollback after the rewrite would possibly result with
+ * buffers for the original filenode having the wrong
+ * persistence setting.
+ *
+ * NB: This relies on swap_relation_files() also swapping the
+ * persistence. That wouldn't work for pg_class, but that can't
+ * be unlogged anyway.
+ */
+ OIDNewHeap = make_new_heap(tab->relid, NewTableSpace, NewAccessMethod,
persistence, lockmode);
+
+ /*
+ * Copy the heap data into the new table with the desired
+ * modifications, and test the current data within the table
+ * against new constraints generated by ALTER TABLE commands.
+ */
+ ATRewriteTable(tab, OIDNewHeap, lockmode);
- /*
- * Copy the heap data into the new table with the desired
- * modifications, and test the current data within the table
- * against new constraints generated by ALTER TABLE commands.
- */
- ATRewriteTable(tab, OIDNewHeap, lockmode);
-
- /*
- * Swap the physical files of the old and new heaps, then rebuild
- * indexes and discard the old heap. We can use RecentXmin for
- * the table's new relfrozenxid because we rewrote all the tuples
- * in ATRewriteTable, so no older Xid remains in the table. Also,
- * we never try to swap toast tables by content, since we have no
- * interest in letting this code work on system catalogs.
- */
- finish_heap_swap(tab->relid, OIDNewHeap,
- false, false, true,
- !OidIsValid(tab->newTableSpace),
- RecentXmin,
- ReadNextMultiXactId(),
- persistence);
-
- InvokeObjectPostAlterHook(RelationRelationId, tab->relid, 0);
+ /*
+ * Swap the physical files of the old and new heaps, then rebuild
+ * indexes and discard the old heap. We can use RecentXmin for
+ * the table's new relfrozenxid because we rewrote all the tuples
+ * in ATRewriteTable, so no older Xid remains in the table. Also,
+ * we never try to swap toast tables by content, since we have no
+ * interest in letting this code work on system catalogs.
+ */
+ finish_heap_swap(tab->relid, OIDNewHeap,
+ false, false, true,
+ !OidIsValid(tab->newTableSpace),
+ RecentXmin,
+ ReadNextMultiXactId(),
+ persistence);
+
+ InvokeObjectPostAlterHook(RelationRelationId, tab->relid, 0);
+ }
+
}
else
{
@@ -14319,6 +14505,146 @@ AlterTableMoveAll(AlterTableMoveAllStmt *stmt)
return new_tablespaceoid;
}
+/*
+ * Alter Table ALL ... SET LOGGED/UNLOGGED
+ *
+ * Allows a user to change persistence of all objects in a given tablespace in
+ * the current database. Objects can be chosen based on the owner of the
+ * object also, to allow users to change persistene only their objects. The
+ * main permissions handling is done by the lower-level change persistence
+ * function.
+ *
+ * All to-be-modified objects are locked first. If NOWAIT is specified and the
+ * lock can't be acquired then we ereport(ERROR).
+ */
+void
+AlterTableSetLoggedAll(AlterTableSetLoggedAllStmt *stmt)
+{
+ List *relations = NIL;
+ ListCell *l;
+ ScanKeyData key[1];
+ Relation rel;
+ TableScanDesc scan;
+ HeapTuple tuple;
+ Oid tablespaceoid;
+ List *role_oids = roleSpecsToIds(NIL);
+
+ /* Ensure we were not asked to change something we can't */
+ if (stmt->objtype != OBJECT_TABLE)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("only tables can be specified")));
+
+ /* Get the tablespace OID */
+ tablespaceoid = get_tablespace_oid(stmt->tablespacename, false);
+
+ /*
+ * Now that the checks are done, check if we should set either to
+ * InvalidOid because it is our database's default tablespace.
+ */
+ if (tablespaceoid == MyDatabaseTableSpace)
+ tablespaceoid = InvalidOid;
+
+ /*
+ * Walk the list of objects in the tablespace to pick up them. This will
+ * only find objects in our database, of course.
+ */
+ ScanKeyInit(&key[0],
+ Anum_pg_class_reltablespace,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(tablespaceoid));
+
+ rel = table_open(RelationRelationId, AccessShareLock);
+ scan = table_beginscan_catalog(rel, 1, key);
+ while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
+ {
+ Form_pg_class relForm = (Form_pg_class) GETSTRUCT(tuple);
+ Oid relOid = relForm->oid;
+
+ /*
+ * Do not pick-up objects in pg_catalog as part of this, if an admin
+ * really wishes to do so, they can issue the individual ALTER
+ * commands directly.
+ *
+ * Also, explicitly avoid any shared tables, temp tables, or TOAST
+ * (TOAST will be changed with the main table).
+ */
+ if (IsCatalogNamespace(relForm->relnamespace) ||
+ relForm->relisshared ||
+ isAnyTempNamespace(relForm->relnamespace) ||
+ IsToastNamespace(relForm->relnamespace))
+ continue;
+
+ /* Only pick up the object type requested */
+ if (relForm->relkind != RELKIND_RELATION)
+ continue;
+
+ /* Check if we are only picking-up objects owned by certain roles */
+ if (role_oids != NIL && !list_member_oid(role_oids, relForm->relowner))
+ continue;
+
+ /*
+ * Handle permissions-checking here since we are locking the tables
+ * and also to avoid doing a bunch of work only to fail part-way. Note
+ * that permissions will also be checked by AlterTableInternal().
+ *
+ * Caller must be considered an owner on the table of which we're going
+ * to change persistence.
+ */
+ if (!pg_class_ownercheck(relOid, GetUserId()))
+ aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(relOid)),
+ NameStr(relForm->relname));
+
+ if (stmt->nowait &&
+ !ConditionalLockRelationOid(relOid, AccessExclusiveLock))
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_IN_USE),
+ errmsg("aborting because lock on relation \"%s.%s\" is not available",
+ get_namespace_name(relForm->relnamespace),
+ NameStr(relForm->relname))));
+ else
+ LockRelationOid(relOid, AccessExclusiveLock);
+
+ /*
+ * Add to our list of objects of which we're going to change
+ * persistence.
+ */
+ relations = lappend_oid(relations, relOid);
+ }
+
+ table_endscan(scan);
+ table_close(rel, AccessShareLock);
+
+ if (relations == NIL)
+ ereport(NOTICE,
+ (errcode(ERRCODE_NO_DATA_FOUND),
+ errmsg("no matching relations in tablespace \"%s\" found",
+ tablespaceoid == InvalidOid ? "(database default)" :
+ get_tablespace_name(tablespaceoid))));
+
+ /*
+ * Everything is locked, loop through and change persistence of all of the
+ * relations.
+ */
+ foreach(l, relations)
+ {
+ List *cmds = NIL;
+ AlterTableCmd *cmd = makeNode(AlterTableCmd);
+
+ if (stmt->logged)
+ cmd->subtype = AT_SetLogged;
+ else
+ cmd->subtype = AT_SetUnLogged;
+
+ cmds = lappend(cmds, cmd);
+
+ EventTriggerAlterTableStart((Node *) stmt);
+ /* OID is set by AlterTableInternal */
+ AlterTableInternal(lfirst_oid(l), cmds, false);
+ EventTriggerAlterTableEnd();
+ }
+}
+
static void
index_copy_data(Relation rel, RelFileNode newrnode)
{
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index df0b747883..55e38cfe3f 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -4269,6 +4269,19 @@ _copyAlterTableMoveAllStmt(const AlterTableMoveAllStmt *from)
return newnode;
}
+static AlterTableSetLoggedAllStmt *
+_copyAlterTableSetLoggedAllStmt(const AlterTableSetLoggedAllStmt *from)
+{
+ AlterTableSetLoggedAllStmt *newnode = makeNode(AlterTableSetLoggedAllStmt);
+
+ COPY_STRING_FIELD(tablespacename);
+ COPY_SCALAR_FIELD(objtype);
+ COPY_SCALAR_FIELD(logged);
+ COPY_SCALAR_FIELD(nowait);
+
+ return newnode;
+}
+
static CreateExtensionStmt *
_copyCreateExtensionStmt(const CreateExtensionStmt *from)
{
@@ -5622,6 +5635,9 @@ copyObjectImpl(const void *from)
case T_AlterTableMoveAllStmt:
retval = _copyAlterTableMoveAllStmt(from);
break;
+ case T_AlterTableSetLoggedAllStmt:
+ retval = _copyAlterTableSetLoggedAllStmt(from);
+ break;
case T_CreateExtensionStmt:
retval = _copyCreateExtensionStmt(from);
break;
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index cb7ddd463c..a19b7874d7 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1917,6 +1917,18 @@ _equalAlterTableMoveAllStmt(const AlterTableMoveAllStmt *a,
}
static bool
+_equalAlterTableSetLoggedAllStmt(const AlterTableSetLoggedAllStmt *a,
+ const AlterTableSetLoggedAllStmt *b)
+{
+ COMPARE_STRING_FIELD(tablespacename);
+ COMPARE_SCALAR_FIELD(objtype);
+ COMPARE_SCALAR_FIELD(logged);
+ COMPARE_SCALAR_FIELD(nowait);
+
+ return true;
+}
+
+static bool
_equalCreateExtensionStmt(const CreateExtensionStmt *a, const CreateExtensionStmt *b)
{
COMPARE_STRING_FIELD(extname);
@@ -3625,6 +3637,9 @@ equal(const void *a, const void *b)
case T_AlterTableMoveAllStmt:
retval = _equalAlterTableMoveAllStmt(a, b);
break;
+ case T_AlterTableSetLoggedAllStmt:
+ retval = _equalAlterTableSetLoggedAllStmt(a, b);
+ break;
case T_CreateExtensionStmt:
retval = _equalCreateExtensionStmt(a, b);
break;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 3d4dd43e47..9823d57a54 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -1984,6 +1984,26 @@ AlterTableStmt:
n->nowait = $13;
$$ = (Node *)n;
}
+ | ALTER TABLE ALL IN_P TABLESPACE name SET LOGGED opt_nowait
+ {
+ AlterTableSetLoggedAllStmt *n =
+ makeNode(AlterTableSetLoggedAllStmt);
+ n->tablespacename = $6;
+ n->objtype = OBJECT_TABLE;
+ n->logged = true;
+ n->nowait = $9;
+ $$ = (Node *)n;
+ }
+ | ALTER TABLE ALL IN_P TABLESPACE name SET UNLOGGED opt_nowait
+ {
+ AlterTableSetLoggedAllStmt *n =
+ makeNode(AlterTableSetLoggedAllStmt);
+ n->tablespacename = $6;
+ n->objtype = OBJECT_TABLE;
+ n->logged = false;
+ n->nowait = $9;
+ $$ = (Node *)n;
+ }
| ALTER INDEX qualified_name alter_table_cmds
{
AlterTableStmt *n = makeNode(AlterTableStmt);
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index ec0485705d..45e1a5d817 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -1070,6 +1070,7 @@ sendDir(bbsink *sink, const char *path, int basepathlen, bool sizeonly,
bool excludeFound;
ForkNumber relForkNum; /* Type of fork if file is a relation */
int relOidChars; /* Chars in filename that are the rel oid */
+ StorageMarks mark;
/* Skip special stuff */
if (strcmp(de->d_name, ".") == 0 || strcmp(de->d_name, "..") == 0)
@@ -1120,7 +1121,7 @@ sendDir(bbsink *sink, const char *path, int basepathlen, bool sizeonly,
/* Exclude all forks for unlogged tables except the init fork */
if (isDbDir &&
parse_filename_for_nontemp_relation(de->d_name, &relOidChars,
- &relForkNum))
+ &relForkNum, &mark))
{
/* Never exclude init forks */
if (relForkNum != INIT_FORKNUM)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index b4532948d3..dab74bf99a 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -37,6 +37,7 @@
#include "access/xlogutils.h"
#include "catalog/catalog.h"
#include "catalog/storage.h"
+#include "catalog/storage_xlog.h"
#include "executor/instrument.h"
#include "lib/binaryheap.h"
#include "miscadmin.h"
@@ -3155,6 +3156,93 @@ DropRelFileNodeBuffers(SMgrRelation smgr_reln, ForkNumber *forkNum,
}
/* ---------------------------------------------------------------------
+ * SetRelFileNodeBuffersPersistence
+ *
+ * This function changes the persistence of all buffer pages of a relation
+ * then writes all dirty pages of the relation out to disk when switching
+ * to PERMANENT. (or more accurately, out to kernel disk buffers),
+ * ensuring that the kernel has an up-to-date view of the relation.
+ *
+ * Generally, the caller should be holding AccessExclusiveLock on the
+ * target relation to ensure that no other backend is busy dirtying
+ * more blocks of the relation; the effects can't be expected to last
+ * after the lock is released.
+ *
+ * XXX currently it sequentially searches the buffer pool, should be
+ * changed to more clever ways of searching. This routine is not
+ * used in any performance-critical code paths, so it's not worth
+ * adding additional overhead to normal paths to make it go faster;
+ * but see also DropRelFileNodeBuffers.
+ * --------------------------------------------------------------------
+ */
+void
+SetRelationBuffersPersistence(SMgrRelation srel, bool permanent, bool isRedo)
+{
+ int i;
+ RelFileNodeBackend rnode = srel->smgr_rnode;
+
+ Assert (!RelFileNodeBackendIsTemp(rnode));
+
+ if (!isRedo)
+ log_smgrbufpersistence(&srel->smgr_rnode.node, permanent);
+
+ ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
+
+ for (i = 0; i < NBuffers; i++)
+ {
+ BufferDesc *bufHdr = GetBufferDescriptor(i);
+ uint32 buf_state;
+
+ if (!RelFileNodeEquals(bufHdr->tag.rnode, rnode.node))
+ continue;
+
+ ReservePrivateRefCountEntry();
+
+ buf_state = LockBufHdr(bufHdr);
+
+ if (!RelFileNodeEquals(bufHdr->tag.rnode, rnode.node))
+ {
+ UnlockBufHdr(bufHdr, buf_state);
+ continue;
+ }
+
+ if (permanent)
+ {
+ /* Init fork is being dropped, drop buffers for it. */
+ if (bufHdr->tag.forkNum == INIT_FORKNUM)
+ {
+ InvalidateBuffer(bufHdr);
+ continue;
+ }
+
+ buf_state |= BM_PERMANENT;
+ pg_atomic_write_u32(&bufHdr->state, buf_state);
+
+ /* we flush this buffer when switching to PERMANENT */
+ if ((buf_state & (BM_VALID | BM_DIRTY)) == (BM_VALID | BM_DIRTY))
+ {
+ PinBuffer_Locked(bufHdr);
+ LWLockAcquire(BufferDescriptorGetContentLock(bufHdr),
+ LW_SHARED);
+ FlushBuffer(bufHdr, srel);
+ LWLockRelease(BufferDescriptorGetContentLock(bufHdr));
+ UnpinBuffer(bufHdr, true);
+ }
+ else
+ UnlockBufHdr(bufHdr, buf_state);
+ }
+ else
+ {
+ /* init fork is always BM_PERMANENT. See BufferAlloc */
+ if (bufHdr->tag.forkNum != INIT_FORKNUM)
+ buf_state &= ~BM_PERMANENT;
+
+ UnlockBufHdr(bufHdr, buf_state);
+ }
+ }
+}
+
+/* ---------------------------------------------------------------------
* DropRelFileNodesAllBuffers
*
* This function removes from the buffer pool all the pages of all
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 263057841d..8487ae1f02 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -349,8 +349,6 @@ static void pre_sync_fname(const char *fname, bool isdir, int elevel);
static void datadir_fsync_fname(const char *fname, bool isdir, int elevel);
static void unlink_if_exists_fname(const char *fname, bool isdir, int elevel);
-static int fsync_parent_path(const char *fname, int elevel);
-
/*
* pg_fsync --- do fsync with or without writethrough
@@ -3759,7 +3757,7 @@ fsync_fname_ext(const char *fname, bool isdir, bool ignore_perm, int elevel)
* This is aimed at making file operations persistent on disk in case of
* an OS crash or power failure.
*/
-static int
+int
fsync_parent_path(const char *fname, int elevel)
{
char parentpath[MAXPGPATH];
diff --git a/src/backend/storage/file/reinit.c b/src/backend/storage/file/reinit.c
index 0ae3fb6902..a34aa8e9af 100644
--- a/src/backend/storage/file/reinit.c
+++ b/src/backend/storage/file/reinit.c
@@ -16,29 +16,49 @@
#include <unistd.h>
+#include "access/xlog.h"
+#include "catalog/pg_tablespace_d.h"
#include "common/relpath.h"
#include "postmaster/startup.h"
+#include "storage/bufmgr.h"
#include "storage/copydir.h"
#include "storage/fd.h"
+#include "storage/md.h"
#include "storage/reinit.h"
+#include "storage/smgr.h"
#include "utils/hsearch.h"
#include "utils/memutils.h"
static void ResetUnloggedRelationsInTablespaceDir(const char *tsdirname,
- int op);
+ Oid tspid, int op);
static void ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname,
- int op);
+ Oid tspid, Oid dbid, int op);
typedef struct
{
Oid reloid; /* hash key */
-} unlogged_relation_entry;
+ bool has_init; /* has INIT fork */
+ bool dirty_init; /* needs to remove INIT fork */
+ bool dirty_all; /* needs to remove all forks */
+} relfile_entry;
/*
- * Reset unlogged relations from before the last restart.
+ * Clean up and reset relation files from before the last restart.
*
- * If op includes UNLOGGED_RELATION_CLEANUP, we remove all forks of any
- * relation with an "init" fork, except for the "init" fork itself.
+ * If op includes UNLOGGED_RELATION_CLEANUP, we perform different operations
+ * depending on the existence of the "cleanup" forks.
+ *
+ * If SMGR_MARK_UNCOMMITTED mark file for init fork is present, we remove the
+ * init fork along with the mark file.
+ *
+ * If SMGR_MARK_UNCOMMITTED mark file for main fork is present we remove the
+ * whole relation along with the mark file.
+ *
+ * Otherwise, if the "init" fork is found. we remove all forks of any relation
+ * with the "init" fork, except for the "init" fork itself.
+ *
+ * If op includes UNLOGGED_RELATION_DROP_BUFFER, we drop all buffers for all
+ * relations that have the "cleanup" and/or the "init" forks.
*
* If op includes UNLOGGED_RELATION_INIT, we copy the "init" fork to the main
* fork.
@@ -72,7 +92,7 @@ ResetUnloggedRelations(int op)
/*
* First process unlogged files in pg_default ($PGDATA/base)
*/
- ResetUnloggedRelationsInTablespaceDir("base", op);
+ ResetUnloggedRelationsInTablespaceDir("base", DEFAULTTABLESPACE_OID, op);
/*
* Cycle through directories for all non-default tablespaces.
@@ -81,13 +101,19 @@ ResetUnloggedRelations(int op)
while ((spc_de = ReadDir(spc_dir, "pg_tblspc")) != NULL)
{
+ Oid tspid;
+
if (strcmp(spc_de->d_name, ".") == 0 ||
strcmp(spc_de->d_name, "..") == 0)
continue;
snprintf(temp_path, sizeof(temp_path), "pg_tblspc/%s/%s",
spc_de->d_name, TABLESPACE_VERSION_DIRECTORY);
- ResetUnloggedRelationsInTablespaceDir(temp_path, op);
+
+ tspid = atooid(spc_de->d_name);
+ Assert(tspid != 0);
+
+ ResetUnloggedRelationsInTablespaceDir(temp_path, tspid, op);
}
FreeDir(spc_dir);
@@ -103,7 +129,8 @@ ResetUnloggedRelations(int op)
* Process one tablespace directory for ResetUnloggedRelations
*/
static void
-ResetUnloggedRelationsInTablespaceDir(const char *tsdirname, int op)
+ResetUnloggedRelationsInTablespaceDir(const char *tsdirname,
+ Oid tspid, int op)
{
DIR *ts_dir;
struct dirent *de;
@@ -130,6 +157,8 @@ ResetUnloggedRelationsInTablespaceDir(const char *tsdirname, int op)
while ((de = ReadDir(ts_dir, tsdirname)) != NULL)
{
+ Oid dbid;
+
/*
* We're only interested in the per-database directories, which have
* numeric names. Note that this code will also (properly) ignore "."
@@ -148,7 +177,9 @@ ResetUnloggedRelationsInTablespaceDir(const char *tsdirname, int op)
ereport_startup_progress("resetting unlogged relations (cleanup), elapsed time: %ld.%02d s, current path: %s",
dbspace_path);
- ResetUnloggedRelationsInDbspaceDir(dbspace_path, op);
+ dbid = atooid(de->d_name);
+ Assert(dbid != 0);
+ ResetUnloggedRelationsInDbspaceDir(dbspace_path, tspid, dbid, op);
}
FreeDir(ts_dir);
@@ -158,125 +189,228 @@ ResetUnloggedRelationsInTablespaceDir(const char *tsdirname, int op)
* Process one per-dbspace directory for ResetUnloggedRelations
*/
static void
-ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname, int op)
+ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname,
+ Oid tspid, Oid dbid, int op)
{
DIR *dbspace_dir;
struct dirent *de;
char rm_path[MAXPGPATH * 2];
+ HTAB *hash;
+ HASHCTL ctl;
/* Caller must specify at least one operation. */
- Assert((op & (UNLOGGED_RELATION_CLEANUP | UNLOGGED_RELATION_INIT)) != 0);
+ Assert((op & (UNLOGGED_RELATION_CLEANUP |
+ UNLOGGED_RELATION_DROP_BUFFER |
+ UNLOGGED_RELATION_INIT)) != 0);
/*
* Cleanup is a two-pass operation. First, we go through and identify all
* the files with init forks. Then, we go through again and nuke
* everything with the same OID except the init fork.
*/
- if ((op & UNLOGGED_RELATION_CLEANUP) != 0)
+
+ /*
+ * It's possible that someone could create a ton of unlogged relations
+ * in the same database & tablespace, so we'd better use a hash table
+ * rather than an array or linked list to keep track of which files
+ * need to be reset. Otherwise, this cleanup operation would be
+ * O(n^2).
+ */
+ memset(&ctl, 0, sizeof(ctl));
+ ctl.keysize = sizeof(Oid);
+ ctl.entrysize = sizeof(relfile_entry);
+ hash = hash_create("relfilenode cleanup hash",
+ 32, &ctl, HASH_ELEM | HASH_BLOBS);
+
+ /* Collect INIT fork and mark files in the directory. */
+ dbspace_dir = AllocateDir(dbspacedirname);
+ while ((de = ReadDir(dbspace_dir, dbspacedirname)) != NULL)
{
- HTAB *hash;
- HASHCTL ctl;
+ int oidchars;
+ ForkNumber forkNum;
+ StorageMarks mark;
- /*
- * It's possible that someone could create a ton of unlogged relations
- * in the same database & tablespace, so we'd better use a hash table
- * rather than an array or linked list to keep track of which files
- * need to be reset. Otherwise, this cleanup operation would be
- * O(n^2).
- */
- ctl.keysize = sizeof(Oid);
- ctl.entrysize = sizeof(unlogged_relation_entry);
- ctl.hcxt = CurrentMemoryContext;
- hash = hash_create("unlogged relation OIDs", 32, &ctl,
- HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
+ /* Skip anything that doesn't look like a relation data file. */
+ if (!parse_filename_for_nontemp_relation(de->d_name, &oidchars,
+ &forkNum, &mark))
+ continue;
- /* Scan the directory. */
- dbspace_dir = AllocateDir(dbspacedirname);
- while ((de = ReadDir(dbspace_dir, dbspacedirname)) != NULL)
+ if (forkNum == INIT_FORKNUM || mark == SMGR_MARK_UNCOMMITTED)
{
- ForkNumber forkNum;
- int oidchars;
- unlogged_relation_entry ent;
-
- /* Skip anything that doesn't look like a relation data file. */
- if (!parse_filename_for_nontemp_relation(de->d_name, &oidchars,
- &forkNum))
- continue;
-
- /* Also skip it unless this is the init fork. */
- if (forkNum != INIT_FORKNUM)
- continue;
+ Oid key;
+ relfile_entry *ent;
+ bool found;
/*
- * Put the OID portion of the name into the hash table, if it
- * isn't already.
+ * Record the relfilenode information. If it has
+ * SMGR_MARK_UNCOMMITTED mark files, the relfilenode is in dirty
+ * state, where clean up is needed.
*/
- ent.reloid = atooid(de->d_name);
- (void) hash_search(hash, &ent, HASH_ENTER, NULL);
+ key = atooid(de->d_name);
+ ent = hash_search(hash, &key, HASH_ENTER, &found);
+
+ if (!found)
+ {
+ ent->has_init = false;
+ ent->dirty_init = false;
+ ent->dirty_all = false;
+ }
+
+ if (forkNum == INIT_FORKNUM && mark == SMGR_MARK_UNCOMMITTED)
+ ent->dirty_init = true;
+ else if (forkNum == MAIN_FORKNUM && mark == SMGR_MARK_UNCOMMITTED)
+ ent->dirty_all = true;
+ else
+ {
+ Assert(forkNum == INIT_FORKNUM);
+ ent->has_init = true;
+ }
}
+ }
- /* Done with the first pass. */
- FreeDir(dbspace_dir);
+ /* Done with the first pass. */
+ FreeDir(dbspace_dir);
+
+ /* nothing to do if we don't have init nor cleanup forks */
+ if (hash_get_num_entries(hash) < 1)
+ {
+ hash_destroy(hash);
+ return;
+ }
+ if ((op & UNLOGGED_RELATION_DROP_BUFFER) != 0)
+ {
/*
- * If we didn't find any init forks, there's no point in continuing;
- * we can bail out now.
+ * When we come here after recovery, smgr object for this file might
+ * have been created. In that case we need to drop all buffers then the
+ * smgr object before initializing the unlogged relation. This is safe
+ * as far as no other backends have accessed the relation before
+ * starting archive recovery.
*/
- if (hash_get_num_entries(hash) == 0)
+ HASH_SEQ_STATUS status;
+ relfile_entry *ent;
+ SMgrRelation *srels = palloc(sizeof(SMgrRelation) * 8);
+ int maxrels = 8;
+ int nrels = 0;
+ int i;
+
+ Assert(!HotStandbyActive());
+
+ hash_seq_init(&status, hash);
+ while((ent = (relfile_entry *) hash_seq_search(&status)) != NULL)
{
- hash_destroy(hash);
- return;
+ RelFileNodeBackend rel;
+
+ /*
+ * The relation is persistent and stays remain persistent. Don't
+ * drop the buffers for this relation.
+ */
+ if (ent->has_init && ent->dirty_init)
+ continue;
+
+ if (maxrels <= nrels)
+ {
+ maxrels *= 2;
+ srels = repalloc(srels, sizeof(SMgrRelation) * maxrels);
+ }
+
+ rel.backend = InvalidBackendId;
+ rel.node.spcNode = tspid;
+ rel.node.dbNode = dbid;
+ rel.node.relNode = ent->reloid;
+
+ srels[nrels++] = smgropen(rel.node, InvalidBackendId);
}
- /*
- * Now, make a second pass and remove anything that matches.
- */
+ DropRelFileNodesAllBuffers(srels, nrels);
+
+ for (i = 0 ; i < nrels ; i++)
+ smgrclose(srels[i]);
+ }
+
+ /*
+ * Now, make a second pass and remove anything that matches.
+ */
+ if ((op & UNLOGGED_RELATION_CLEANUP) != 0)
+ {
dbspace_dir = AllocateDir(dbspacedirname);
while ((de = ReadDir(dbspace_dir, dbspacedirname)) != NULL)
{
- ForkNumber forkNum;
- int oidchars;
- unlogged_relation_entry ent;
+ ForkNumber forkNum;
+ StorageMarks mark;
+ int oidchars;
+ Oid key;
+ relfile_entry *ent;
+ RelFileNodeBackend rel;
/* Skip anything that doesn't look like a relation data file. */
if (!parse_filename_for_nontemp_relation(de->d_name, &oidchars,
- &forkNum))
- continue;
-
- /* We never remove the init fork. */
- if (forkNum == INIT_FORKNUM)
+ &forkNum, &mark))
continue;
/*
* See whether the OID portion of the name shows up in the hash
* table. If so, nuke it!
*/
- ent.reloid = atooid(de->d_name);
- if (hash_search(hash, &ent, HASH_FIND, NULL))
+ key = atooid(de->d_name);
+ ent = hash_search(hash, &key, HASH_FIND, NULL);
+
+ if (!ent)
+ continue;
+
+ if (!ent->dirty_all)
{
- snprintf(rm_path, sizeof(rm_path), "%s/%s",
- dbspacedirname, de->d_name);
- if (unlink(rm_path) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m",
- rm_path)));
+ /* clean permanent relations don't need cleanup */
+ if (!ent->has_init)
+ continue;
+
+ if (ent->dirty_init)
+ {
+ /*
+ * The crashed trasaction did SET UNLOGGED. This relation
+ * is restored to a LOGGED relation.
+ */
+ if (forkNum != INIT_FORKNUM)
+ continue;
+ }
else
- elog(DEBUG2, "unlinked file \"%s\"", rm_path);
+ {
+ /*
+ * we don't remove the INIT fork of a non-dirty
+ * relfilenode
+ */
+ if (forkNum == INIT_FORKNUM && mark == SMGR_MARK_NONE)
+ continue;
+ }
}
+
+ /* so, nuke it! */
+ snprintf(rm_path, sizeof(rm_path), "%s/%s",
+ dbspacedirname, de->d_name);
+ if (unlink(rm_path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m",
+ rm_path)));
+
+ rel.backend = InvalidBackendId;
+ rel.node.spcNode = tspid;
+ rel.node.dbNode = dbid;
+ rel.node.relNode = atooid(de->d_name);
+
+ ForgetRelationForkSyncRequests(rel, forkNum);
}
/* Cleanup is complete. */
FreeDir(dbspace_dir);
- hash_destroy(hash);
}
+ hash_destroy(hash);
+ hash = NULL;
+
/*
* Initialization happens after cleanup is complete: we copy each init
- * fork file to the corresponding main fork file. Note that if we are
- * asked to do both cleanup and init, we may never get here: if the
- * cleanup code determines that there are no init forks in this dbspace,
- * it will return before we get to this point.
+ * fork file to the corresponding main fork file.
*/
if ((op & UNLOGGED_RELATION_INIT) != 0)
{
@@ -285,6 +419,7 @@ ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname, int op)
while ((de = ReadDir(dbspace_dir, dbspacedirname)) != NULL)
{
ForkNumber forkNum;
+ StorageMarks mark;
int oidchars;
char oidbuf[OIDCHARS + 1];
char srcpath[MAXPGPATH * 2];
@@ -292,9 +427,11 @@ ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname, int op)
/* Skip anything that doesn't look like a relation data file. */
if (!parse_filename_for_nontemp_relation(de->d_name, &oidchars,
- &forkNum))
+ &forkNum, &mark))
continue;
+ Assert(mark == SMGR_MARK_NONE);
+
/* Also skip it unless this is the init fork. */
if (forkNum != INIT_FORKNUM)
continue;
@@ -328,15 +465,18 @@ ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname, int op)
while ((de = ReadDir(dbspace_dir, dbspacedirname)) != NULL)
{
ForkNumber forkNum;
+ StorageMarks mark;
int oidchars;
char oidbuf[OIDCHARS + 1];
char mainpath[MAXPGPATH];
/* Skip anything that doesn't look like a relation data file. */
if (!parse_filename_for_nontemp_relation(de->d_name, &oidchars,
- &forkNum))
+ &forkNum, &mark))
continue;
+ Assert(mark == SMGR_MARK_NONE);
+
/* Also skip it unless this is the init fork. */
if (forkNum != INIT_FORKNUM)
continue;
@@ -379,7 +519,7 @@ ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname, int op)
*/
bool
parse_filename_for_nontemp_relation(const char *name, int *oidchars,
- ForkNumber *fork)
+ ForkNumber *fork, StorageMarks *mark)
{
int pos;
@@ -410,11 +550,19 @@ parse_filename_for_nontemp_relation(const char *name, int *oidchars,
for (segchar = 1; isdigit((unsigned char) name[pos + segchar]); ++segchar)
;
- if (segchar <= 1)
- return false;
- pos += segchar;
+ if (segchar > 1)
+ pos += segchar;
}
+ /* mark file? */
+ if (name[pos] == '.' && name[pos + 1] != 0)
+ {
+ *mark = name[pos + 1];
+ pos += 2;
+ }
+ else
+ *mark = SMGR_MARK_NONE;
+
/* Now we should be at the end. */
if (name[pos] != '\0')
return false;
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index b4bca7eed6..27ca9c1ca2 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -139,7 +139,8 @@ static MdfdVec *_mdfd_getseg(SMgrRelation reln, ForkNumber forkno,
BlockNumber blkno, bool skipFsync, int behavior);
static BlockNumber _mdnblocks(SMgrRelation reln, ForkNumber forknum,
MdfdVec *seg);
-
+static bool mdmarkexists(SMgrRelation reln, ForkNumber forkNum,
+ StorageMarks mark);
/*
* mdinit() -- Initialize private state for magnetic disk storage manager.
@@ -170,6 +171,80 @@ mdexists(SMgrRelation reln, ForkNumber forkNum)
}
/*
+ * mdcreatemark() -- Create a mark file.
+ *
+ * If isRedo is true, it's okay for the file to exist already.
+ */
+void
+mdcreatemark(SMgrRelation reln, ForkNumber forkNum, StorageMarks mark,
+ bool isRedo)
+{
+ char *path =markpath(reln->smgr_rnode, forkNum, mark);
+ int fd;
+
+ /* See mdcreate for details.. */
+ TablespaceCreateDbspace(reln->smgr_rnode.node.spcNode,
+ reln->smgr_rnode.node.dbNode,
+ isRedo);
+
+ fd = BasicOpenFile(path, O_WRONLY | O_CREAT | O_EXCL);
+ if (fd < 0 && (!isRedo || errno != EEXIST))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not crete mark file \"%s\": %m", path)));
+
+ pfree(path);
+ pg_fsync(fd);
+ close(fd);
+
+ /*
+ * To guarantee that the creation of the file is persistent, fsync its
+ * parent directory.
+ */
+ fsync_parent_path(path, ERROR);
+}
+
+
+/*
+ * mdunlinkmark() -- Delete the mark file
+ *
+ * If isRedo is true, it's okay for the file being not found.
+ */
+void
+mdunlinkmark(SMgrRelation reln, ForkNumber forkNum, StorageMarks mark,
+ bool isRedo)
+{
+ char *path = markpath(reln->smgr_rnode, forkNum, mark);
+
+ if (!isRedo || mdmarkexists(reln, forkNum, mark))
+ durable_unlink(path, ERROR);
+
+ pfree(path);
+}
+
+/*
+ * mdmarkexists() -- Check if the file exists.
+ */
+static bool
+mdmarkexists(SMgrRelation reln, ForkNumber forkNum, StorageMarks mark)
+{
+ char *path = markpath(reln->smgr_rnode, forkNum, mark);
+ int fd;
+
+ fd = BasicOpenFile(path, O_RDONLY);
+ if (fd < 0 && errno != ENOENT)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not access mark file \"%s\": %m", path)));
+ pfree(path);
+
+ if (fd < 0)
+ return false;
+
+ return true;
+}
+
+/*
* mdcreate() -- Create a new relation on magnetic disk.
*
* If isRedo is true, it's okay for the relation to exist already.
@@ -1026,6 +1101,15 @@ register_forget_request(RelFileNodeBackend rnode, ForkNumber forknum,
}
/*
+ * ForgetRelationForkSyncRequests -- forget any fsyncs and unlinks for a fork
+ */
+void
+ForgetRelationForkSyncRequests(RelFileNodeBackend rnode, ForkNumber forknum)
+{
+ register_forget_request(rnode, forknum, 0);
+}
+
+/*
* ForgetDatabaseSyncRequests -- forget any fsyncs and unlinks for a DB
*/
void
@@ -1378,12 +1462,14 @@ mdsyncfiletag(const FileTag *ftag, char *path)
* Return 0 on success, -1 on failure, with errno set.
*/
int
-mdunlinkfiletag(const FileTag *ftag, char *path)
+mdunlinkfiletag(const FileTag *ftag, char *path, StorageMarks mark)
{
char *p;
/* Compute the path. */
- p = relpathperm(ftag->rnode, MAIN_FORKNUM);
+ p = GetRelationPath(ftag->rnode.dbNode, ftag->rnode.spcNode,
+ ftag->rnode.relNode, InvalidBackendId, MAIN_FORKNUM,
+ mark);
strlcpy(path, p, MAXPGPATH);
pfree(p);
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index 0fcef4994b..110e64b0b2 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -62,6 +62,10 @@ typedef struct f_smgr
void (*smgr_truncate) (SMgrRelation reln, ForkNumber forknum,
BlockNumber nblocks);
void (*smgr_immedsync) (SMgrRelation reln, ForkNumber forknum);
+ void (*smgr_createmark) (SMgrRelation reln, ForkNumber forknum,
+ StorageMarks mark, bool isRedo);
+ void (*smgr_unlinkmark) (SMgrRelation reln, ForkNumber forknum,
+ StorageMarks mark, bool isRedo);
} f_smgr;
static const f_smgr smgrsw[] = {
@@ -82,6 +86,8 @@ static const f_smgr smgrsw[] = {
.smgr_nblocks = mdnblocks,
.smgr_truncate = mdtruncate,
.smgr_immedsync = mdimmedsync,
+ .smgr_createmark = mdcreatemark,
+ .smgr_unlinkmark = mdunlinkmark,
}
};
@@ -336,6 +342,26 @@ smgrcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo)
}
/*
+ * smgrcreatemark() -- Create a mark file
+ */
+void
+smgrcreatemark(SMgrRelation reln, ForkNumber forknum, StorageMarks mark,
+ bool isRedo)
+{
+ smgrsw[reln->smgr_which].smgr_createmark(reln, forknum, mark, isRedo);
+}
+
+/*
+ * smgrunlinkmark() -- Delete a mark file
+ */
+void
+smgrunlinkmark(SMgrRelation reln, ForkNumber forknum, StorageMarks mark,
+ bool isRedo)
+{
+ smgrsw[reln->smgr_which].smgr_unlinkmark(reln, forknum, mark, isRedo);
+}
+
+/*
* smgrdosyncall() -- Immediately sync all forks of all given relations
*
* All forks of all given relations are synced out to the store.
@@ -662,6 +688,12 @@ smgrimmedsync(SMgrRelation reln, ForkNumber forknum)
smgrsw[reln->smgr_which].smgr_immedsync(reln, forknum);
}
+void
+smgrunlink(SMgrRelation reln, ForkNumber forknum, bool isRedo)
+{
+ smgrsw[reln->smgr_which].smgr_unlink(reln->smgr_rnode, forknum, isRedo);
+}
+
/*
* AtEOXact_SMgr
*
diff --git a/src/backend/storage/sync/sync.c b/src/backend/storage/sync/sync.c
index d4083e8a56..9563940d45 100644
--- a/src/backend/storage/sync/sync.c
+++ b/src/backend/storage/sync/sync.c
@@ -89,7 +89,8 @@ static CycleCtr checkpoint_cycle_ctr = 0;
typedef struct SyncOps
{
int (*sync_syncfiletag) (const FileTag *ftag, char *path);
- int (*sync_unlinkfiletag) (const FileTag *ftag, char *path);
+ int (*sync_unlinkfiletag) (const FileTag *ftag, char *path,
+ StorageMarks mark);
bool (*sync_filetagmatches) (const FileTag *ftag,
const FileTag *candidate);
} SyncOps;
@@ -222,7 +223,8 @@ SyncPostCheckpoint(void)
/* Unlink the file */
if (syncsw[entry->tag.handler].sync_unlinkfiletag(&entry->tag,
- path) < 0)
+ path,
+ SMGR_MARK_NONE) < 0)
{
/*
* There's a race condition, when the database is dropped at the
@@ -236,6 +238,20 @@ SyncPostCheckpoint(void)
(errcode_for_file_access(),
errmsg("could not remove file \"%s\": %m", path)));
}
+ else if (syncsw[entry->tag.handler].sync_unlinkfiletag(
+ &entry->tag, path,
+ SMGR_MARK_UNCOMMITTED) < 0)
+ {
+ /*
+ * And we may have SMGR_MARK_UNCOMMITTED file. Remove it if the
+ * fork files has been successfully removed. It's ok if the file
+ * does not exist.
+ */
+ if (errno != ENOENT)
+ ereport(WARNING,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+ }
/* Mark the list entry as canceled, just in case */
entry->canceled = true;
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 1fbc387d47..1483f9a475 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -162,6 +162,7 @@ ClassifyUtilityCommandAsReadOnly(Node *parsetree)
case T_AlterTSConfigurationStmt:
case T_AlterTSDictionaryStmt:
case T_AlterTableMoveAllStmt:
+ case T_AlterTableSetLoggedAllStmt:
case T_AlterTableSpaceOptionsStmt:
case T_AlterTableStmt:
case T_AlterTypeStmt:
@@ -1747,6 +1748,12 @@ ProcessUtilitySlow(ParseState *pstate,
commandCollected = true;
break;
+ case T_AlterTableSetLoggedAllStmt:
+ AlterTableSetLoggedAll((AlterTableSetLoggedAllStmt *) parsetree);
+ /* commands are stashed in AlterTableSetLoggedAll */
+ commandCollected = true;
+ break;
+
case T_DropStmt:
ExecDropStmt((DropStmt *) parsetree, isTopLevel);
/* no commands stashed for DROP */
@@ -2669,6 +2676,10 @@ CreateCommandTag(Node *parsetree)
tag = AlterObjectTypeCommandTag(((AlterTableMoveAllStmt *) parsetree)->objtype);
break;
+ case T_AlterTableSetLoggedAllStmt:
+ tag = AlterObjectTypeCommandTag(((AlterTableSetLoggedAllStmt *) parsetree)->objtype);
+ break;
+
case T_AlterTableStmt:
tag = AlterObjectTypeCommandTag(((AlterTableStmt *) parsetree)->objtype);
break;
diff --git a/src/bin/pg_rewind/parsexlog.c b/src/bin/pg_rewind/parsexlog.c
index 436df54120..dbc0da5da5 100644
--- a/src/bin/pg_rewind/parsexlog.c
+++ b/src/bin/pg_rewind/parsexlog.c
@@ -407,6 +407,30 @@ extractPageInfo(XLogReaderState *record)
* source system.
*/
}
+ else if (rmid == RM_SMGR_ID && rminfo == XLOG_SMGR_UNLINK)
+ {
+ /*
+ * We can safely ignore these. When we compare the sizes later on,
+ * we'll notice that they differ, and copy the missing tail from
+ * source system.
+ */
+ }
+ else if (rmid == RM_SMGR_ID && rminfo == XLOG_SMGR_MARK)
+ {
+ /*
+ * We can safely ignore these. When we compare the sizes later on,
+ * we'll notice that they differ, and copy the missing tail from
+ * source system.
+ */
+ }
+ else if (rmid == RM_SMGR_ID && rminfo == XLOG_SMGR_BUFPERSISTENCE)
+ {
+ /*
+ * We can safely ignore these. When we compare the sizes later on,
+ * we'll notice that they differ, and copy the missing tail from
+ * source system.
+ */
+ }
else if (rmid == RM_XACT_ID &&
((rminfo & XLOG_XACT_OPMASK) == XLOG_XACT_COMMIT ||
(rminfo & XLOG_XACT_OPMASK) == XLOG_XACT_COMMIT_PREPARED ||
diff --git a/src/common/relpath.c b/src/common/relpath.c
index 1f5c426ec0..67f24890d6 100644
--- a/src/common/relpath.c
+++ b/src/common/relpath.c
@@ -139,9 +139,15 @@ GetDatabasePath(Oid dbNode, Oid spcNode)
*/
char *
GetRelationPath(Oid dbNode, Oid spcNode, Oid relNode,
- int backendId, ForkNumber forkNumber)
+ int backendId, ForkNumber forkNumber, char mark)
{
char *path;
+ char markstr[10];
+
+ if (mark == 0)
+ markstr[0] = 0;
+ else
+ snprintf(markstr, 10, ".%c", mark);
if (spcNode == GLOBALTABLESPACE_OID)
{
@@ -149,10 +155,10 @@ GetRelationPath(Oid dbNode, Oid spcNode, Oid relNode,
Assert(dbNode == 0);
Assert(backendId == InvalidBackendId);
if (forkNumber != MAIN_FORKNUM)
- path = psprintf("global/%u_%s",
- relNode, forkNames[forkNumber]);
+ path = psprintf("global/%u_%s%s",
+ relNode, forkNames[forkNumber], markstr);
else
- path = psprintf("global/%u", relNode);
+ path = psprintf("global/%u%s", relNode, markstr);
}
else if (spcNode == DEFAULTTABLESPACE_OID)
{
@@ -160,22 +166,22 @@ GetRelationPath(Oid dbNode, Oid spcNode, Oid relNode,
if (backendId == InvalidBackendId)
{
if (forkNumber != MAIN_FORKNUM)
- path = psprintf("base/%u/%u_%s",
+ path = psprintf("base/%u/%u_%s%s",
dbNode, relNode,
- forkNames[forkNumber]);
+ forkNames[forkNumber], markstr);
else
- path = psprintf("base/%u/%u",
- dbNode, relNode);
+ path = psprintf("base/%u/%u%s",
+ dbNode, relNode, markstr);
}
else
{
if (forkNumber != MAIN_FORKNUM)
- path = psprintf("base/%u/t%d_%u_%s",
+ path = psprintf("base/%u/t%d_%u_%s%s",
dbNode, backendId, relNode,
- forkNames[forkNumber]);
+ forkNames[forkNumber], markstr);
else
- path = psprintf("base/%u/t%d_%u",
- dbNode, backendId, relNode);
+ path = psprintf("base/%u/t%d_%u%s",
+ dbNode, backendId, relNode, markstr);
}
}
else
@@ -184,27 +190,28 @@ GetRelationPath(Oid dbNode, Oid spcNode, Oid relNode,
if (backendId == InvalidBackendId)
{
if (forkNumber != MAIN_FORKNUM)
- path = psprintf("pg_tblspc/%u/%s/%u/%u_%s",
+ path = psprintf("pg_tblspc/%u/%s/%u/%u_%s%s",
spcNode, TABLESPACE_VERSION_DIRECTORY,
dbNode, relNode,
- forkNames[forkNumber]);
+ forkNames[forkNumber], markstr);
else
- path = psprintf("pg_tblspc/%u/%s/%u/%u",
+ path = psprintf("pg_tblspc/%u/%s/%u/%u%s",
spcNode, TABLESPACE_VERSION_DIRECTORY,
- dbNode, relNode);
+ dbNode, relNode, markstr);
}
else
{
if (forkNumber != MAIN_FORKNUM)
- path = psprintf("pg_tblspc/%u/%s/%u/t%d_%u_%s",
+ path = psprintf("pg_tblspc/%u/%s/%u/t%d_%u_%s%s",
spcNode, TABLESPACE_VERSION_DIRECTORY,
dbNode, backendId, relNode,
- forkNames[forkNumber]);
+ forkNames[forkNumber], markstr);
else
- path = psprintf("pg_tblspc/%u/%s/%u/t%d_%u",
+ path = psprintf("pg_tblspc/%u/%s/%u/t%d_%u%s",
spcNode, TABLESPACE_VERSION_DIRECTORY,
- dbNode, backendId, relNode);
+ dbNode, backendId, relNode, markstr);
}
}
+
return path;
}
diff --git a/src/include/catalog/storage.h b/src/include/catalog/storage.h
index 0ab32b44e9..382623159c 100644
--- a/src/include/catalog/storage.h
+++ b/src/include/catalog/storage.h
@@ -23,6 +23,8 @@
extern int wal_skip_threshold;
extern SMgrRelation RelationCreateStorage(RelFileNode rnode, char relpersistence);
+extern void RelationCreateInitFork(Relation rel);
+extern void RelationDropInitFork(Relation rel);
extern void RelationDropStorage(Relation rel);
extern void RelationPreserveStorage(RelFileNode rnode, bool atCommit);
extern void RelationPreTruncate(Relation rel);
diff --git a/src/include/catalog/storage_xlog.h b/src/include/catalog/storage_xlog.h
index f0814f1458..12346ed7f6 100644
--- a/src/include/catalog/storage_xlog.h
+++ b/src/include/catalog/storage_xlog.h
@@ -18,17 +18,23 @@
#include "lib/stringinfo.h"
#include "storage/block.h"
#include "storage/relfilenode.h"
+#include "storage/smgr.h"
/*
* Declarations for smgr-related XLOG records
*
- * Note: we log file creation and truncation here, but logging of deletion
- * actions is handled by xact.c, because it is part of transaction commit.
+ * Note: we log file creation, truncation and buffer persistence change here,
+ * but logging of deletion actions is handled mainly by xact.c, because it is
+ * part of transaction commit in most cases. However, there's a case where
+ * init forks are deleted outside control of transaction.
*/
/* XLOG gives us high 4 bits */
#define XLOG_SMGR_CREATE 0x10
#define XLOG_SMGR_TRUNCATE 0x20
+#define XLOG_SMGR_UNLINK 0x30
+#define XLOG_SMGR_MARK 0x40
+#define XLOG_SMGR_BUFPERSISTENCE 0x50
typedef struct xl_smgr_create
{
@@ -36,6 +42,32 @@ typedef struct xl_smgr_create
ForkNumber forkNum;
} xl_smgr_create;
+typedef struct xl_smgr_unlink
+{
+ RelFileNode rnode;
+ ForkNumber forkNum;
+} xl_smgr_unlink;
+
+typedef enum smgr_mark_action
+{
+ XLOG_SMGR_MARK_CREATE = 'c',
+ XLOG_SMGR_MARK_UNLINK = 'u'
+} smgr_mark_action;
+
+typedef struct xl_smgr_mark
+{
+ RelFileNode rnode;
+ ForkNumber forkNum;
+ StorageMarks mark;
+ smgr_mark_action action;
+} xl_smgr_mark;
+
+typedef struct xl_smgr_bufpersistence
+{
+ RelFileNode rnode;
+ bool persistence;
+} xl_smgr_bufpersistence;
+
/* flags for xl_smgr_truncate */
#define SMGR_TRUNCATE_HEAP 0x0001
#define SMGR_TRUNCATE_VM 0x0002
@@ -51,6 +83,12 @@ typedef struct xl_smgr_truncate
} xl_smgr_truncate;
extern void log_smgrcreate(const RelFileNode *rnode, ForkNumber forkNum);
+extern void log_smgrunlink(const RelFileNode *rnode, ForkNumber forkNum);
+extern void log_smgrcreatemark(const RelFileNode *rnode, ForkNumber forkNum,
+ StorageMarks mark);
+extern void log_smgrunlinkmark(const RelFileNode *rnode, ForkNumber forkNum,
+ StorageMarks mark);
+extern void log_smgrbufpersistence(const RelFileNode *rnode, bool persistence);
extern void smgr_redo(XLogReaderState *record);
extern void smgr_desc(StringInfo buf, XLogReaderState *record);
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index 336549cc5f..714077ff4c 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -42,6 +42,8 @@ extern void AlterTableInternal(Oid relid, List *cmds, bool recurse);
extern Oid AlterTableMoveAll(AlterTableMoveAllStmt *stmt);
+extern void AlterTableSetLoggedAll(AlterTableSetLoggedAllStmt *stmt);
+
extern ObjectAddress AlterTableNamespace(AlterObjectSchemaStmt *stmt,
Oid *oldschema);
diff --git a/src/include/common/relpath.h b/src/include/common/relpath.h
index a44be11ca0..106a5cf508 100644
--- a/src/include/common/relpath.h
+++ b/src/include/common/relpath.h
@@ -67,7 +67,7 @@ extern int forkname_chars(const char *str, ForkNumber *fork);
extern char *GetDatabasePath(Oid dbNode, Oid spcNode);
extern char *GetRelationPath(Oid dbNode, Oid spcNode, Oid relNode,
- int backendId, ForkNumber forkNumber);
+ int backendId, ForkNumber forkNumber, char mark);
/*
* Wrapper macros for GetRelationPath. Beware of multiple
@@ -77,7 +77,7 @@ extern char *GetRelationPath(Oid dbNode, Oid spcNode, Oid relNode,
/* First argument is a RelFileNode */
#define relpathbackend(rnode, backend, forknum) \
GetRelationPath((rnode).dbNode, (rnode).spcNode, (rnode).relNode, \
- backend, forknum)
+ backend, forknum, 0)
/* First argument is a RelFileNode */
#define relpathperm(rnode, forknum) \
@@ -87,4 +87,9 @@ extern char *GetRelationPath(Oid dbNode, Oid spcNode, Oid relNode,
#define relpath(rnode, forknum) \
relpathbackend((rnode).node, (rnode).backend, forknum)
+/* First argument is a RelFileNodeBackend */
+#define markpath(rnode, forknum, mark) \
+ GetRelationPath((rnode).node.dbNode, (rnode).node.spcNode, \
+ (rnode).node.relNode, \
+ (rnode).backend, forknum, mark)
#endif /* RELPATH_H */
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index 7c657c1241..8860b2e548 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -428,6 +428,7 @@ typedef enum NodeTag
T_AlterCollationStmt,
T_CallStmt,
T_AlterStatsStmt,
+ T_AlterTableSetLoggedAllStmt,
/*
* TAGS FOR PARSE TREE NODES (parsenodes.h)
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 4c5a8a39bf..c3e1bc66d1 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2350,6 +2350,15 @@ typedef struct AlterTableMoveAllStmt
bool nowait;
} AlterTableMoveAllStmt;
+typedef struct AlterTableSetLoggedAllStmt
+{
+ NodeTag type;
+ char *tablespacename;
+ ObjectType objtype; /* Object type to move */
+ bool logged;
+ bool nowait;
+} AlterTableSetLoggedAllStmt;
+
/* ----------------------
* Create/Alter Extension Statements
* ----------------------
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index cfce23ecbc..f5a7df87a4 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -206,6 +206,8 @@ extern void FlushRelationsAllBuffers(struct SMgrRelationData **smgrs, int nrels)
extern void FlushDatabaseBuffers(Oid dbid);
extern void DropRelFileNodeBuffers(struct SMgrRelationData *smgr_reln, ForkNumber *forkNum,
int nforks, BlockNumber *firstDelBlock);
+extern void SetRelationBuffersPersistence(struct SMgrRelationData *srel,
+ bool permanent, bool isRedo);
extern void DropRelFileNodesAllBuffers(struct SMgrRelationData **smgr_reln, int nnodes);
extern void DropDatabaseBuffers(Oid dbid);
diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h
index 34602ae006..2dc0357ad5 100644
--- a/src/include/storage/fd.h
+++ b/src/include/storage/fd.h
@@ -185,6 +185,7 @@ extern ssize_t pg_pwritev_with_retry(int fd,
extern int pg_truncate(const char *path, off_t length);
extern void fsync_fname(const char *fname, bool isdir);
extern int fsync_fname_ext(const char *fname, bool isdir, bool ignore_perm, int elevel);
+extern int fsync_parent_path(const char *fname, int elevel);
extern int durable_rename(const char *oldfile, const char *newfile, int loglevel);
extern int durable_unlink(const char *fname, int loglevel);
extern int durable_rename_excl(const char *oldfile, const char *newfile, int loglevel);
diff --git a/src/include/storage/md.h b/src/include/storage/md.h
index 752b440864..99620816b5 100644
--- a/src/include/storage/md.h
+++ b/src/include/storage/md.h
@@ -23,6 +23,10 @@
extern void mdinit(void);
extern void mdopen(SMgrRelation reln);
extern void mdclose(SMgrRelation reln, ForkNumber forknum);
+extern void mdcreatemark(SMgrRelation reln, ForkNumber forknum,
+ StorageMarks mark, bool isRedo);
+extern void mdunlinkmark(SMgrRelation reln, ForkNumber forknum,
+ StorageMarks mark, bool isRedo);
extern void mdcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo);
extern bool mdexists(SMgrRelation reln, ForkNumber forknum);
extern void mdunlink(RelFileNodeBackend rnode, ForkNumber forknum, bool isRedo);
@@ -41,12 +45,14 @@ extern void mdtruncate(SMgrRelation reln, ForkNumber forknum,
BlockNumber nblocks);
extern void mdimmedsync(SMgrRelation reln, ForkNumber forknum);
+extern void ForgetRelationForkSyncRequests(RelFileNodeBackend rnode,
+ ForkNumber forknum);
extern void ForgetDatabaseSyncRequests(Oid dbid);
extern void DropRelationFiles(RelFileNode *delrels, int ndelrels, bool isRedo);
/* md sync callbacks */
extern int mdsyncfiletag(const FileTag *ftag, char *path);
-extern int mdunlinkfiletag(const FileTag *ftag, char *path);
+extern int mdunlinkfiletag(const FileTag *ftag, char *path, StorageMarks mark);
extern bool mdfiletagmatches(const FileTag *ftag, const FileTag *candidate);
#endif /* MD_H */
diff --git a/src/include/storage/reinit.h b/src/include/storage/reinit.h
index fad1e5c473..e1f97e9b89 100644
--- a/src/include/storage/reinit.h
+++ b/src/include/storage/reinit.h
@@ -16,13 +16,15 @@
#define REINIT_H
#include "common/relpath.h"
-
+#include "storage/smgr.h"
extern void ResetUnloggedRelations(int op);
-extern bool parse_filename_for_nontemp_relation(const char *name,
- int *oidchars, ForkNumber *fork);
+extern bool parse_filename_for_nontemp_relation(const char *name, int *oidchars,
+ ForkNumber *fork,
+ StorageMarks *mark);
#define UNLOGGED_RELATION_CLEANUP 0x0001
-#define UNLOGGED_RELATION_INIT 0x0002
+#define UNLOGGED_RELATION_DROP_BUFFER 0x0002
+#define UNLOGGED_RELATION_INIT 0x0004
#endif /* REINIT_H */
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index a6fbf7b6a6..201ecace8a 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -19,6 +19,18 @@
#include "storage/relfilenode.h"
/*
+ * Storage marks is a file of which existence suggests something about a
+ * file. The name of such files is "<filename>.<mark>", where the mark is one
+ * of the values of StorageMarks. Since ".<digit>" means segment files so don't
+ * use digits for the mark character.
+ */
+typedef enum StorageMarks
+{
+ SMGR_MARK_NONE = 0,
+ SMGR_MARK_UNCOMMITTED = 'u' /* the file is not committed yet */
+} StorageMarks;
+
+/*
* smgr.c maintains a table of SMgrRelation objects, which are essentially
* cached file handles. An SMgrRelation is created (if not already present)
* by smgropen(), and destroyed by smgrclose(). Note that neither of these
@@ -85,7 +97,12 @@ extern void smgrclearowner(SMgrRelation *owner, SMgrRelation reln);
extern void smgrclose(SMgrRelation reln);
extern void smgrcloseall(void);
extern void smgrclosenode(RelFileNodeBackend rnode);
+extern void smgrcreatemark(SMgrRelation reln, ForkNumber forknum,
+ StorageMarks mark, bool isRedo);
+extern void smgrunlinkmark(SMgrRelation reln, ForkNumber forknum,
+ StorageMarks mark, bool isRedo);
extern void smgrcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo);
+extern void smgrunlink(SMgrRelation reln, ForkNumber forknum, bool isRedo);
extern void smgrdosyncall(SMgrRelation *rels, int nrels);
extern void smgrdounlinkall(SMgrRelation *rels, int nrels, bool isRedo);
extern void smgrextend(SMgrRelation reln, ForkNumber forknum,
--
2.11.0
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2021-12-17 12:46 Justin Pryzby <[email protected]>
parent: Jakub Wartak <[email protected]>
1 sibling, 1 reply; 52+ messages in thread
From: Justin Pryzby @ 2021-12-17 12:46 UTC (permalink / raw)
To: Jakub Wartak <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
On Fri, Dec 17, 2021 at 09:10:30AM +0000, Jakub Wartak wrote:
> I'm especially worried if I didn't screw up something/forgot something related to the last one (rd->rd_smgr changes), but I'm getting "All 210 tests passed".
> As the thread didn't get a lot of traction, I've registered it into current commitfest https://commitfest.postgresql.org/36/3461/ with You as the author and in 'Ready for review' state.
> I think it behaves as almost finished one and apparently after reading all those discussions that go back over 10years+ time span about this feature, and lot of failed effort towards wal_level=noWAL I think it would be nice to finally start getting some of that of it into the core.
The patch is failing:
http://cfbot.cputube.org/kyotaro-horiguchi.html
https://api.cirrus-ci.com/v1/artifact/task/5564333871595520/regress_diffs/src/bin/pg_upgrade/tmp_che...
I think you ran "make check", but should run something like this:
make check-world -j8 >check-world.log 2>&1 && echo Success
--
Justin
^ permalink raw reply [nested|flat] 52+ messages in thread
* RE: In-placre persistance change of a relation
@ 2021-12-17 14:33 Jakub Wartak <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Jakub Wartak @ 2021-12-17 14:33 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>
> Justin wrote:
> On Fri, Dec 17, 2021 at 09:10:30AM +0000, Jakub Wartak wrote:
> > As the thread didn't get a lot of traction, I've registered it into current
> commitfest
> https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcommitf
> est.postgresql.org%2F36%2F3461%2F&data=04%7C01%7CJakub.Wartak%
> 40tomtom.com%7Cb815e75090d44e20fd0a08d9c15b45cc%7C374f80267b544a
> 3ab87d328fa26ec10d%7C0%7C0%7C637753420044612362%7CUnknown%7CT
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXV
> CI6Mn0%3D%7C3000&sdata=0BTQSVDnVPu4YpECKXXlBJT5q3Gfgv099SaC
> NuBwiW4%3D&reserved=0 with You as the author and in 'Ready for
> review' state.
>
> The patch is failing:
[..]
> I think you ran "make check", but should run something like this:
> make check-world -j8 >check-world.log 2>&1 && echo Success
Hi Justin,
I've repeated the check-world and it says to me all is ok locally (also with --enable-cassert --enable-debug , at least on Amazon Linux 2) and also installcheck on default params seems to be ok
I don't seem to understand why testfarm reports errors for tests like "path, polygon, rowsecurity" e.g. on Linux/graviton2 and FreeBSD while not on MacOS(?) .
Could someone point to me where to start looking/fixing?
-J.
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2021-12-17 19:10 Justin Pryzby <[email protected]>
parent: Jakub Wartak <[email protected]>
0 siblings, 0 replies; 52+ messages in thread
From: Justin Pryzby @ 2021-12-17 19:10 UTC (permalink / raw)
To: Jakub Wartak <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
On Fri, Dec 17, 2021 at 02:33:25PM +0000, Jakub Wartak wrote:
> > Justin wrote:
> > On Fri, Dec 17, 2021 at 09:10:30AM +0000, Jakub Wartak wrote:
> > > As the thread didn't get a lot of traction, I've registered it into current
> > commitfest
> > https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcommitf
> > est.postgresql.org%2F36%2F3461%2F&data=04%7C01%7CJakub.Wartak%
> > 40tomtom.com%7Cb815e75090d44e20fd0a08d9c15b45cc%7C374f80267b544a
> > 3ab87d328fa26ec10d%7C0%7C0%7C637753420044612362%7CUnknown%7CT
> > WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXV
> > CI6Mn0%3D%7C3000&sdata=0BTQSVDnVPu4YpECKXXlBJT5q3Gfgv099SaC
> > NuBwiW4%3D&reserved=0 with You as the author and in 'Ready for
> > review' state.
> >
> > The patch is failing:
> [..]
> > I think you ran "make check", but should run something like this:
> > make check-world -j8 >check-world.log 2>&1 && echo Success
>
> Hi Justin,
>
> I've repeated the check-world and it says to me all is ok locally (also with --enable-cassert --enable-debug , at least on Amazon Linux 2) and also installcheck on default params seems to be ok
> I don't seem to understand why testfarm reports errors for tests like "path, polygon, rowsecurity" e.g. on Linux/graviton2 and FreeBSD while not on MacOS(?) .
> Could someone point to me where to start looking/fixing?
Since it says this, it looks a lot like a memory error like a use-after-free
- like in fsync_parent_path():
CREATE TABLE PATH_TBL (f1 path);
+ERROR: could not open file <....> Pacific": No such file or directory
I see at least this one is still failing, though:
time make -C src/test/recovery check
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2021-12-20 06:28 Kyotaro Horiguchi <[email protected]>
parent: Jakub Wartak <[email protected]>
1 sibling, 2 replies; 52+ messages in thread
From: Kyotaro Horiguchi @ 2021-12-20 06:28 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Hello, Jakub.
At Fri, 17 Dec 2021 09:10:30 +0000, Jakub Wartak <[email protected]> wrote in
> the patch didn't apply clean (it's from March; some hunks were failing), so I've fixed it and the combined git format-patch is attached. It did conflict with the following:
Thanks for looking this. Also thanks for Justin for finding the silly
use-after-free bug. (Now I see the regression test fails and I'm not
sure how come I didn't find this before.)
> b0483263dda - Add support for SET ACCESS METHOD in ALTER TABLE
> 7b565843a94 - Add call to object access hook at the end of table rewrite in ALTER TABLE
> 9ce346eabf3 - Report progress of startup operations that take a long time.
> f10f0ae420 - Replace RelationOpenSmgr() with RelationGetSmgr().
>
> I'm especially worried if I didn't screw up something/forgot something related to the last one (rd->rd_smgr changes), but I'm getting "All 210 tests passed".
About the last one, all rel->rd_smgr acesses need to be repalced with
RelationGetSmgr(). On the other hand we can simply remove
RelationOpenSmgr() calls since the target smgrrelation is guaranteed
to be loaded by RelationGetSmgr().
The fix you made for RelationCreate/DropInitFork is correct and
changes you made would work, but I prefer that the code not being too
permissive for unknown (or unexpected) states.
> Basic demonstration of this patch (with wal_level=minimal):
> create unlogged table t6 (id bigint, t text);
> -- produces 110GB table, takes ~5mins
> insert into t6 select nextval('s1'), repeat('A', 1000) from generate_series(1, 100000000);
> alter table t6 set logged;
> on baseline SET LOGGED takes: ~7min10s
> on patched SET LOGGED takes: 25s
>
> So basically one can - thanks to this patch - use his application (performing classic INSERTs/UPDATEs/DELETEs, so without the need to rewrite to use COPY) and perform literally batch upload and then just switch the tables to LOGGED.
This result is significant. That operation finally requires WAL writes
but I was not sure how much gain FPIs (or bulk WAL logging) gives in
comparison to operational WALs.
> Some more intensive testing also looks good, assuming table prepared to put pressure on WAL:
> create unlogged table t_unlogged (id bigint, t text) partition by hash (id);
> create unlogged table t_unlogged_h0 partition of t_unlogged FOR VALUES WITH (modulus 4, remainder 0);
> [..]
> create unlogged table t_unlogged_h3 partition of t_unlogged FOR VALUES WITH (modulus 4, remainder 3);
>
> Workload would still be pretty heavy on LWLock/BufferContent,WALInsert and Lock/extend .
> t_logged.sql = insert into t_logged select nextval('s1'), repeat('A', 1000) from generate_series(1, 1000); # according to pg_wal_stats.wal_bytes generates ~1MB of WAL
> t_unlogged.sql = insert into t_unlogged select nextval('s1'), repeat('A', 1000) from generate_series(1, 1000); # according to pg_wal_stats.wal_bytes generates ~3kB of WAL
>
> so using: pgbench -f <tabletypetest>.sql -T 30 -P 1 -c 32 -j 3 t
> with synchronous_commit =ON(default):
> with t_logged.sql: tps = 229 (lat avg = 138ms)
> with t_unlogged.sql tps = 283 (lat avg = 112ms) # almost all on LWLock/WALWrite
> with synchronous_commit =OFF:
> with t_logged.sql: tps = 413 (lat avg = 77ms)
> with t_unloged.sql: tps = 782 (lat avg = 40ms)
> Afterwards switching the unlogged ~16GB partitions takes 5s per partition.
>
> As the thread didn't get a lot of traction, I've registered it into current commitfest https://commitfest.postgresql.org/36/3461/ with You as the author and in 'Ready for review' state.
>
> I think it behaves as almost finished one and apparently after reading all those discussions that go back over 10years+ time span about this feature, and lot of failed effort towards wal_level=noWAL I think it would be nice to finally start getting some of that of it into the core.
Thanks for taking the performance benchmark.
I didn't register for some reasons.
1. I'm not sure that we want to have the new mark files.
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.
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).
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2021-12-20 07:53 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
1 sibling, 0 replies; 52+ messages in thread
From: Kyotaro Horiguchi @ 2021-12-20 07:53 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Mon, 20 Dec 2021 15:28:23 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
>
> 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 issue 4 above is not fixed (yet).
Not only for the case, RelationChangePersistence needs to send a
truncate record before FPIs. If primary crashes amid of the
operation, the table content will be vanish with the persistence
change. That is the correct behavior.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* RE: In-placre persistance change of a relation
@ 2021-12-20 07:59 Jakub Wartak <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
1 sibling, 1 reply; 52+ messages in thread
From: Jakub Wartak @ 2021-12-20 07:59 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>
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.
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2021-12-20 08:39 Kyotaro Horiguchi <[email protected]>
parent: Jakub Wartak <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2021-12-20 08:39 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Mon, 20 Dec 2021 07:59:29 +0000, Jakub Wartak <[email protected]> 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
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2021-12-20 08:52 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2021-12-20 08:52 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Mon, 20 Dec 2021 17:39:27 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> At Mon, 20 Dec 2021 07:59:29 +0000, Jakub Wartak <[email protected]> wrote in
> > 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!
That crash vanished after a recompilation for me and I don't see that
error. On my dev env node# 349 is T_ALterTableSetLoggedAllStmt, which
0002 adds. So perhaps make clean/make all would fix that.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* RE: In-placre persistance change of a relation
@ 2021-12-20 13:38 Jakub Wartak <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Jakub Wartak @ 2021-12-20 13:38 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>
Hi Kyotaro,
> At Mon, 20 Dec 2021 17:39:27 +0900 (JST), Kyotaro Horiguchi
> <[email protected]> wrote in
> > At Mon, 20 Dec 2021 07:59:29 +0000, Jakub Wartak
> > <[email protected]> wrote in
> > > 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!
>
> That crash vanished after a recompilation for me and I don't see that error. On
> my dev env node# 349 is T_ALterTableSetLoggedAllStmt, which
> 0002 adds. So perhaps make clean/make all would fix that.
The fastest I could - I've repeated the whole cycle about that one with fresh v9 (make clean, configure, make install, fresh initdb) and I've found two problems:
1) check-worlds seems OK but make -C src/test/recovery check shows a couple of failing tests here locally and in https://cirrus-ci.com/task/4699985735319552?logs=test#L807 :
t/009_twophase.pl (Wstat: 256 Tests: 24 Failed: 1)
Failed test: 21
Non-zero exit status: 1
t/014_unlogged_reinit.pl (Wstat: 512 Tests: 12 Failed: 2)
Failed tests: 9-10
Non-zero exit status: 2
t/018_wal_optimize.pl (Wstat: 7424 Tests: 0 Failed: 0)
Non-zero exit status: 29
Parse errors: Bad plan. You planned 38 tests but ran 0.
t/022_crash_temp_files.pl (Wstat: 7424 Tests: 6 Failed: 0)
Non-zero exit status: 29
Parse errors: Bad plan. You planned 9 tests but ran 6.
018 made no sense, I've tried to take a quick look with wal_level=minimal why it is failing , it is mystery to me as the sequence seems to be pretty basic but the outcome is not:
~> cat repro.sql
create tablespace tbs1 location '/tbs1';
CREATE TABLE moved (id int);
INSERT INTO moved VALUES (1);
BEGIN;
ALTER TABLE moved SET TABLESPACE tbs1;
CREATE TABLE originated (id int);
INSERT INTO originated VALUES (1);
CREATE UNIQUE INDEX ON originated(id) TABLESPACE tbs1;
COMMIT;
~> psql -f repro.sql z3; sleep 1; /usr/pgsql-15/bin/pg_ctl -D /var/lib/pgsql/15/data -l logfile -m immediate stop
CREATE TABLESPACE
CREATE TABLE
INSERT 0 1
BEGIN
ALTER TABLE
CREATE TABLE
INSERT 0 1
CREATE INDEX
COMMIT
waiting for server to shut down.... done
server stopped
~> /usr/pgsql-15/bin/pg_ctl -D /var/lib/pgsql/15/data -l logfile start
waiting for server to start.... done
server started
z3# select * from moved;
ERROR: could not open file "pg_tblspc/32834/PG_15_202112131/32833/32838": No such file or directory
z3=# select * from originated;
ERROR: could not open file "base/32833/32839": No such file or directory
z3=# \dt+
List of relations
Schema | Name | Type | Owner | Persistence | Size | Description
--------+------------+-------+----------+-------------+---------+-------------
public | moved | table | postgres | permanent | 0 bytes |
public | originated | table | postgres | permanent | 0 bytes |
This happens even without placing on tablespace at all {for originated table , but no for moved on}, some major mishap is there (commit should guarantee correctness) or I'm tired and having sloppy fingers.
2) minor one testcase, still something is odd.
drop tablespace tbs1;
create tablespace tbs1 location '/tbs1';
CREATE UNLOGGED TABLE t4 (a int) tablespace tbs1;
CREATE UNLOGGED TABLE t5 (a int) tablespace tbs1;
CREATE UNLOGGED TABLE t6 (a int) tablespace tbs1;
CREATE TABLE t7 (a int) tablespace tbs1;
insert into t7 values (1);
insert into t5 values (1);
insert into t6 values (1);
\dt+
List of relations
Schema | Name | Type | Owner | Persistence | Size | Description
--------+------+-------+----------+-------------+------------+-------------
public | t4 | table | postgres | unlogged | 0 bytes |
public | t5 | table | postgres | unlogged | 8192 bytes |
public | t6 | table | postgres | unlogged | 8192 bytes |
public | t7 | table | postgres | permanent | 8192 bytes |
(4 rows)
ALTER TABLE ALL IN TABLESPACE tbs1 set logged;
==> STILL WARNING: unrecognized node type: 349
\dt+
List of relations
Schema | Name | Type | Owner | Persistence | Size | Description
--------+------+-------+----------+-------------+------------+-------------
public | t4 | table | postgres | permanent | 0 bytes |
public | t5 | table | postgres | permanent | 8192 bytes |
public | t6 | table | postgres | permanent | 8192 bytes |
public | t7 | table | postgres | permanent | 8192 bytes |
So it did rewrite however this warning seems to be unfixed. I've tested on e2c52beecdea152ca680a22ef35c6a7da55aa30f.
-J.
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2021-12-21 08:13 Kyotaro Horiguchi <[email protected]>
parent: Jakub Wartak <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2021-12-21 08:13 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Ugh! I completely forgot about TAP tests.. Thanks for the testing and
sorry for the bugs.
This is a bit big change so I need a bit of time before posting the
next version.
At Mon, 20 Dec 2021 13:38:35 +0000, Jakub Wartak <[email protected]> wrote in
> 1) check-worlds seems OK but make -C src/test/recovery check shows a couple of failing tests here locally and in https://cirrus-ci.com/task/4699985735319552?logs=test#L807 :
> t/009_twophase.pl (Wstat: 256 Tests: 24 Failed: 1)
> Failed test: 21
> Non-zero exit status: 1
PREPARE TRANSACTION requires uncommited file creation to be
committed. Concretely we need to remove the "mark" files for the
in-transaction created relation file during PREPARE TRANSACTION.
pendingSync is not a parallel mechanism with pendingDeletes so we
cannot move mark deletion to pendingSync.
After all I decided to add a separate list pendingCleanups for pending
non-deletion tasks separately from pendingDeletes and execute it
before insering the commit record. Not only the above but also all of
the following failures vanished by the change.
> t/014_unlogged_reinit.pl (Wstat: 512 Tests: 12 Failed: 2)
> Failed tests: 9-10
> Non-zero exit status: 2
> t/018_wal_optimize.pl (Wstat: 7424 Tests: 0 Failed: 0)
> Non-zero exit status: 29
> Parse errors: Bad plan. You planned 38 tests but ran 0.
> t/022_crash_temp_files.pl (Wstat: 7424 Tests: 6 Failed: 0)
> Non-zero exit status: 29
> Parse errors: Bad plan. You planned 9 tests but ran 6.
> 018 made no sense, I've tried to take a quick look with wal_level=minimal why it is failing , it is mystery to me as the sequence seems to be pretty basic but the outcome is not:
I think this shares the same cause.
> ~> cat repro.sql
> create tablespace tbs1 location '/tbs1';
> CREATE TABLE moved (id int);
> INSERT INTO moved VALUES (1);
> BEGIN;
> ALTER TABLE moved SET TABLESPACE tbs1;
> CREATE TABLE originated (id int);
> INSERT INTO originated VALUES (1);
> CREATE UNIQUE INDEX ON originated(id) TABLESPACE tbs1;
> COMMIT;
..
> ERROR: could not open file "base/32833/32839": No such file or directory
> z3=# \dt+
> List of relations
> Schema | Name | Type | Owner | Persistence | Size | Description
> --------+------------+-------+----------+-------------+---------+-------------
> public | moved | table | postgres | permanent | 0 bytes |
> public | originated | table | postgres | permanent | 0 bytes |
>
> This happens even without placing on tablespace at all {for originated table , but no for moved on}, some major mishap is there (commit should guarantee correctness) or I'm tired and having sloppy fingers.
>
> 2) minor one testcase, still something is odd.
>
> drop tablespace tbs1;
> create tablespace tbs1 location '/tbs1';
> CREATE UNLOGGED TABLE t4 (a int) tablespace tbs1;
> CREATE UNLOGGED TABLE t5 (a int) tablespace tbs1;
> CREATE UNLOGGED TABLE t6 (a int) tablespace tbs1;
> CREATE TABLE t7 (a int) tablespace tbs1;
> insert into t7 values (1);
> insert into t5 values (1);
> insert into t6 values (1);
> \dt+
> List of relations
> Schema | Name | Type | Owner | Persistence | Size | Description
> --------+------+-------+----------+-------------+------------+-------------
> public | t4 | table | postgres | unlogged | 0 bytes |
> public | t5 | table | postgres | unlogged | 8192 bytes |
> public | t6 | table | postgres | unlogged | 8192 bytes |
> public | t7 | table | postgres | permanent | 8192 bytes |
> (4 rows)
>
> ALTER TABLE ALL IN TABLESPACE tbs1 set logged;
> ==> STILL WARNING: unrecognized node type: 349
> \dt+
> List of relations
> Schema | Name | Type | Owner | Persistence | Size | Description
> --------+------+-------+----------+-------------+------------+-------------
> public | t4 | table | postgres | permanent | 0 bytes |
> public | t5 | table | postgres | permanent | 8192 bytes |
> public | t6 | table | postgres | permanent | 8192 bytes |
> public | t7 | table | postgres | permanent | 8192 bytes |
>
> So it did rewrite however this warning seems to be unfixed. I've tested on e2c52beecdea152ca680a22ef35c6a7da55aa30f.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2021-12-21 11:04 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2021-12-21 11:04 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Tue, 21 Dec 2021 17:13:21 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> Ugh! I completely forgot about TAP tests.. Thanks for the testing and
> sorry for the bugs.
>
> This is a bit big change so I need a bit of time before posting the
> next version.
I took a bit too long detour but the patch gets to pass make-world for
me.
In this version:
- When relation persistence is changed from logged to unlogged, buffer
persistence is flipped then an init-fork is created along with a mark
file for the fork (RelationCreateInitFork). The mark file is removed
at commit but left alone after a crash before commit. At the next
startup, ResetUnloggedRelationsInDbspaceDir() removes the init fork
file if it finds the mark file corresponding to the file.
- When relation persistence is changed from unlogged to logged, buffer
persistence is flipped then the exisging init-fork is marked to be
dropped at commit (RelationDropInitFork). Finally the whole content
is WAL-logged in the page-wise manner (RelationChangePersistence),
- The two operations above are repeatable within a transaction and
commit makes the last operation persist and rollback make the all
operations abandoned.
- Storage files are created along with a "mark" file for the
relfilenode. It behaves the same way to the above except the mark
files corresponds to the whole relfilenode.
- The at-commit operations this patch adds require to be WAL-logged so
they don't fit pendingDeletes list, which is executed after commit. I
added a new pending-work list pendingCleanups that is executed just
after pendingSyncs. (new in this version)
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* RE: In-placre persistance change of a relation
@ 2021-12-21 13:07 Jakub Wartak <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Jakub Wartak @ 2021-12-21 13:07 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>
Hi Kyotaro,
> I took a bit too long detour but the patch gets to pass make-world for me.
Good news, v10 passes all the tests for me (including TAP recover ones). There's major problem I think:
drop table t6;
create unlogged table t6 (id bigint, t text);
create sequence s1;
insert into t6 select nextval('s1'), repeat('A', 1000) from generate_series(1, 100);
alter table t6 set logged;
select pg_sleep(1);
<--optional checkpoint, more on this later.
/usr/pgsql-15/bin/pg_ctl -D /var/lib/pgsql/15/data -l logfile -m immediate stop
/usr/pgsql-15/bin/pg_ctl -D /var/lib/pgsql/15/data -l logfile start
select count(*) from t6; -- shows 0 rows
But If I perform checkpoint before crash, data is there.. apparently the missing steps done by checkpointer
seem to help. If checkpoint is not done, then some peeking reveals that upon startup there is truncation (?!):
$ /usr/pgsql-15/bin/pg_ctl -D /var/lib/pgsql/15/data -l logfile -m immediate stop
$ find /var/lib/pgsql/15/data/ -name '73741*' -ls
112723206 120 -rw------- 1 postgres postgres 122880 Dec 21 12:42 /var/lib/pgsql/15/data/base/73740/73741
112723202 24 -rw------- 1 postgres postgres 24576 Dec 21 12:42 /var/lib/pgsql/15/data/base/73740/73741_fsm
$ /usr/pgsql-15/bin/pg_ctl -D /var/lib/pgsql/15/data -l logfile start
waiting for server to start.... done
server started
$ find /var/lib/pgsql/15/data/ -name '73741*' -ls
112723206 0 -rw------- 1 postgres postgres 0 Dec 21 12:42 /var/lib/pgsql/15/data/base/73740/73741
112723202 16 -rw------- 1 postgres postgres 16384 Dec 21 12:42 /var/lib/pgsql/15/data/base/73740/73741_fsm
So what's suspicious is that 122880 -> 0 file size truncation. I've investigated WAL and it seems to contain TRUNCATE records
after logged FPI images, so when the crash recovery would kick in it probably clears this table (while it shouldn't).
However if I perform CHECKPOINT just before crash the WAL stream contains just RUNNING_XACTS and CHECKPOINT_ONLINE
redo records, this probably prevents truncating. I'm newbie here so please take this theory with grain of salt, it can be
something completely different.
-J.
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2021-12-22 06:13 Kyotaro Horiguchi <[email protected]>
parent: Jakub Wartak <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2021-12-22 06:13 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Hello, Jakub.
At Tue, 21 Dec 2021 13:07:28 +0000, Jakub Wartak <[email protected]> wrote in
> So what's suspicious is that 122880 -> 0 file size truncation. I've investigated WAL and it seems to contain TRUNCATE records
> after logged FPI images, so when the crash recovery would kick in it probably clears this table (while it shouldn't).
Darn.. It is too silly that I wrongly issued truncate records for the
target relation of the function (rel) instaed of the relation on which
we're currently operating at that time (r).
> However if I perform CHECKPOINT just before crash the WAL stream contains just RUNNING_XACTS and CHECKPOINT_ONLINE
> redo records, this probably prevents truncating. I'm newbie here so please take this theory with grain of salt, it can be
> something completely different.
It is because the WAL records are inconsistent with the on-disk state.
After a crash before a checkpoint after the SET LOGGED, recovery ends with
recoverying the broken WAL records, but after that the on-disk state
is persisted and the broken WAL records are not replayed.
The following fix works.
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5478,7 +5478,7 @@ RelationChangePersistence(AlteredTableInfo *tab, char persistence,
xl_smgr_truncate xlrec;
xlrec.blkno = 0;
- xlrec.rnode = rel->rd_node;
+ xlrec.rnode = r->rd_node;
xlrec.flags = SMGR_TRUNCATE_ALL;
I made another change in this version. Previously only btree among all
index AMs was processed in the in-place manner. In this version we do
that all AMs except GiST. Maybe if gistGetFakeLSN behaved the same
way for permanent and unlogged indexes, we could skip index rebuild in
exchange of some extra WAL records emitted while it is unlogged.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* RE: In-placre persistance change of a relation
@ 2021-12-22 08:42 Jakub Wartak <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Jakub Wartak @ 2021-12-22 08:42 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>
Hi Kyotaro,
> At Tue, 21 Dec 2021 13:07:28 +0000, Jakub Wartak
> <[email protected]> wrote in
> > So what's suspicious is that 122880 -> 0 file size truncation. I've
> > investigated WAL and it seems to contain TRUNCATE records after logged
> FPI images, so when the crash recovery would kick in it probably clears this
> table (while it shouldn't).
>
> Darn.. It is too silly that I wrongly issued truncate records for the target
> relation of the function (rel) instaed of the relation on which we're currently
> operating at that time (r).
>
> [..]
> The following fix works.
Cool, I have verified basic stuff that was coming to my mind, now even cfbot is happy with v11, You should happy too I hope :)
> I made another change in this version. Previously only btree among all index
> AMs was processed in the in-place manner. In this version we do that all
> AMs except GiST. Maybe if gistGetFakeLSN behaved the same way for
> permanent and unlogged indexes, we could skip index rebuild in exchange of
> some extra WAL records emitted while it is unlogged.
I think there's slight omission:
-- unlogged table -> logged with GiST:
DROP TABLE IF EXISTS testcase;
CREATE UNLOGGED TABLE testcase(geom geometry not null);
CREATE INDEX idx_testcase_gist ON testcase USING gist(geom);
INSERT INTO testcase(geom) SELECT ST_Buffer(ST_SetSRID(ST_MakePoint(-1.0, 2.0),4326), 0.0001);
ALTER TABLE testcase SET LOGGED;
-- crashes with:
(gdb) where
#0 reindex_index (indexId=indexId@entry=65541, skip_constraint_checks=skip_constraint_checks@entry=true, persistence=persistence@entry=112 'p', params=params@entry=0x0) at index.c:3521
#1 0x000000000062f494 in RelationChangePersistence (tab=tab@entry=0x1947258, persistence=112 'p', lockmode=lockmode@entry=8) at tablecmds.c:5434
#2 0x0000000000642819 in ATRewriteTables (context=0x7ffc19c04520, lockmode=<optimized out>, wqueue=0x7ffc19c04388, parsetree=0x1925ec8) at tablecmds.c:5644
[..]
#10 0x00000000007f078f in exec_simple_query (query_string=0x1925340 "ALTER TABLE testcase SET LOGGED;") at postgres.c:1215
apparently reindex_index() params cannot be NULL - the same happens with switching persistent
table to unlogged one too (with GiST).
I'll also try to give another shot to the patch early next year - as we are starting long Christmas/holiday break here
- with verifying WAL for GiST and more advanced setup (more crashes, and standby/archiving/barman to see
how it's possible to use wal_level=minimal <-> replica transitions).
-J.
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2021-12-23 06:01 Kyotaro Horiguchi <[email protected]>
parent: Jakub Wartak <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2021-12-23 06:01 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Wed, 22 Dec 2021 08:42:14 +0000, Jakub Wartak <[email protected]> wrote in
> I think there's slight omission:
...
> apparently reindex_index() params cannot be NULL - the same happens with switching persistent
Hmm. a3dc926009 has changed the interface. (But the name is also
changed after that.)
-reindex_relation(Oid relid, int flags, int options)
+reindex_relation(Oid relid, int flags, ReindexParams *params)
> I'll also try to give another shot to the patch early next year - as we are starting long Christmas/holiday break here
> - with verifying WAL for GiST and more advanced setup (more crashes, and standby/archiving/barman to see
> how it's possible to use wal_level=minimal <-> replica transitions).
Thanks. I added TAP test to excecise the in-place persistence change.
have a nice holiday, Jakub!
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2021-12-23 06:33 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 52+ messages in thread
From: Kyotaro Horiguchi @ 2021-12-23 06:33 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Thu, 23 Dec 2021 15:01:41 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> I added TAP test to excecise the in-place persistence change.
We don't need a base table for every index. TAP test revised.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* [PATCH v13 05/20] meson: prereq: Add src/tools/gen_export.pl
@ 2022-01-20 07:36 Andres Freund <[email protected]>
0 siblings, 0 replies; 52+ messages in thread
From: Andres Freund @ 2022-01-20 07:36 UTC (permalink / raw)
Currently the logic is all in src/Makefile.shlib. This adds a sketch of a
generation script that can be used from meson.
Author: Andres Freund <[email protected]>
Reviewed-By: Peter Eisentraut <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/tools/gen_export.pl | 81 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 81 insertions(+)
create mode 100644 src/tools/gen_export.pl
diff --git a/src/tools/gen_export.pl b/src/tools/gen_export.pl
new file mode 100644
index 00000000000..68b3ab86614
--- /dev/null
+++ b/src/tools/gen_export.pl
@@ -0,0 +1,81 @@
+use strict;
+use warnings;
+use Getopt::Long;
+
+my $format;
+my $libname;
+my $input;
+my $output;
+
+GetOptions(
+ 'format:s' => \$format,
+ 'libname:s' => \$libname,
+ 'input:s' => \$input,
+ 'output:s' => \$output) or die "wrong arguments";
+
+if (not ($format eq 'aix' or $format eq 'darwin' or $format eq 'gnu' or $format eq 'win'))
+{
+ die "$0: $format is not yet handled (only aix, darwin, gnu, win are)\n";
+}
+
+open(my $input_handle, '<', $input)
+ or die "$0: could not open input file '$input': $!\n";
+
+open(my $output_handle, '>', $output)
+ or die "$0: could not open output file '$output': $!\n";
+
+
+if ($format eq 'gnu')
+{
+ print $output_handle "{
+ global:
+";
+}
+elsif ($format eq 'win')
+{
+ # XXX: Looks like specifying LIBRARY $libname is optional, which makes it
+ # easier to build a generic command for generating export files...
+ if ($libname)
+ {
+ print $output_handle "LIBRARY $libname\n";
+ }
+ print $output_handle "EXPORTS\n";
+}
+
+while (<$input_handle>)
+{
+ if (/^#/)
+ {
+ # don't do anything with a comment
+ }
+ elsif (/^(\S+)\s+(\S+)/)
+ {
+ if ($format eq 'aix')
+ {
+ print $output_handle "$1\n";
+ }
+ elsif ($format eq 'darwin')
+ {
+ print $output_handle "_$1\n";
+ }
+ elsif ($format eq 'gnu')
+ {
+ print $output_handle " $1;\n";
+ }
+ elsif ($format eq 'win')
+ {
+ print $output_handle "$1 @ $2\n";
+ }
+ }
+ else
+ {
+ die "$0: unexpected line $_\n";
+ }
+}
+
+if ($format eq 'gnu')
+{
+ print $output_handle " local: *;
+};
+";
+}
--
2.37.3.542.gdd3f6c4cae
--4uoxke3bx6ymzqvl
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v13-0006-meson-prereq-Refactor-PG_TEST_EXTRA-logic-in-aut.patch"
^ permalink raw reply [nested|flat] 52+ messages in thread
* [PATCH v13 05/20] meson: prereq: Add src/tools/gen_export.pl
@ 2022-01-20 07:36 Andres Freund <[email protected]>
0 siblings, 0 replies; 52+ messages in thread
From: Andres Freund @ 2022-01-20 07:36 UTC (permalink / raw)
Currently the logic is all in src/Makefile.shlib. This adds a sketch of a
generation script that can be used from meson.
Author: Andres Freund <[email protected]>
Reviewed-By: Peter Eisentraut <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/tools/gen_export.pl | 81 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 81 insertions(+)
create mode 100644 src/tools/gen_export.pl
diff --git a/src/tools/gen_export.pl b/src/tools/gen_export.pl
new file mode 100644
index 00000000000..68b3ab86614
--- /dev/null
+++ b/src/tools/gen_export.pl
@@ -0,0 +1,81 @@
+use strict;
+use warnings;
+use Getopt::Long;
+
+my $format;
+my $libname;
+my $input;
+my $output;
+
+GetOptions(
+ 'format:s' => \$format,
+ 'libname:s' => \$libname,
+ 'input:s' => \$input,
+ 'output:s' => \$output) or die "wrong arguments";
+
+if (not ($format eq 'aix' or $format eq 'darwin' or $format eq 'gnu' or $format eq 'win'))
+{
+ die "$0: $format is not yet handled (only aix, darwin, gnu, win are)\n";
+}
+
+open(my $input_handle, '<', $input)
+ or die "$0: could not open input file '$input': $!\n";
+
+open(my $output_handle, '>', $output)
+ or die "$0: could not open output file '$output': $!\n";
+
+
+if ($format eq 'gnu')
+{
+ print $output_handle "{
+ global:
+";
+}
+elsif ($format eq 'win')
+{
+ # XXX: Looks like specifying LIBRARY $libname is optional, which makes it
+ # easier to build a generic command for generating export files...
+ if ($libname)
+ {
+ print $output_handle "LIBRARY $libname\n";
+ }
+ print $output_handle "EXPORTS\n";
+}
+
+while (<$input_handle>)
+{
+ if (/^#/)
+ {
+ # don't do anything with a comment
+ }
+ elsif (/^(\S+)\s+(\S+)/)
+ {
+ if ($format eq 'aix')
+ {
+ print $output_handle "$1\n";
+ }
+ elsif ($format eq 'darwin')
+ {
+ print $output_handle "_$1\n";
+ }
+ elsif ($format eq 'gnu')
+ {
+ print $output_handle " $1;\n";
+ }
+ elsif ($format eq 'win')
+ {
+ print $output_handle "$1 @ $2\n";
+ }
+ }
+ else
+ {
+ die "$0: unexpected line $_\n";
+ }
+}
+
+if ($format eq 'gnu')
+{
+ print $output_handle " local: *;
+};
+";
+}
--
2.37.3.542.gdd3f6c4cae
--4uoxke3bx6ymzqvl
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v13-0006-meson-prereq-Refactor-PG_TEST_EXTRA-logic-in-aut.patch"
^ permalink raw reply [nested|flat] 52+ messages in thread
* [PATCH v11 4/9] meson: prereq: Add src/tools/gen_export.pl
@ 2022-01-20 07:36 Andres Freund <[email protected]>
0 siblings, 0 replies; 52+ messages in thread
From: Andres Freund @ 2022-01-20 07:36 UTC (permalink / raw)
Currently the logic is all in src/Makefile.shlib. This adds a sketch of a
generation script that can be used from meson.
---
src/tools/gen_export.pl | 83 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 83 insertions(+)
create mode 100644 src/tools/gen_export.pl
diff --git a/src/tools/gen_export.pl b/src/tools/gen_export.pl
new file mode 100644
index 00000000000..6a8b196ad89
--- /dev/null
+++ b/src/tools/gen_export.pl
@@ -0,0 +1,83 @@
+use strict;
+use warnings;
+use Getopt::Long;
+
+my $format;
+my $libname;
+my $input;
+my $output;
+
+GetOptions(
+ 'format:s' => \$format,
+ 'libname:s' => \$libname,
+ 'input:s' => \$input,
+ 'output:s' => \$output) or die "wrong arguments";
+
+if (not ($format eq 'aix' or $format eq 'darwin' or $format eq 'gnu' or $format eq 'win'))
+{
+ die "$0: $format is not yet handled (only aix, darwin, gnu, win are)\n";
+}
+
+open(my $input_handle, '<', $input)
+ or die "$0: could not open input file '$input': $!\n";
+
+open(my $output_handle, '>', $output)
+ or die "$0: could not open output file '$output': $!\n";
+
+
+if ($format eq 'gnu')
+{
+ print $output_handle "{
+ global:
+";
+}
+elsif ($format eq 'win')
+{
+ # XXX: Looks like specifying LIBRARY $libname is optional, which makes it
+ # easier to build a generic command for generating export files...
+ if ($libname)
+ {
+ print $output_handle "LIBRARY $libname\n";
+ }
+ print $output_handle "EXPORTS\n";
+}
+
+while (<$input_handle>)
+{
+ if (/^#/)
+ {
+ # don't do anything with a comment
+ }
+ elsif (/^([^\s]+)\s+([^\s]+)/)
+ {
+ if ($format eq 'aix')
+ {
+ print $output_handle " $1\n";
+ }
+ elsif ($format eq 'darwin')
+ {
+ print $output_handle " _$1\n";
+ }
+ elsif ($format eq 'gnu')
+ {
+ print $output_handle " $1;\n";
+ }
+ elsif ($format eq 'win')
+ {
+ print $output_handle " $1 @ $2\n";
+ }
+ }
+ else
+ {
+ die "$0: unexpected line $_\n";
+ }
+}
+
+if ($format eq 'gnu')
+{
+ print $output_handle " local: *;
+};
+";
+}
+
+exit(0);
--
2.37.0.3.g30cc8d0f14
--epod4qqm5dwbci5r
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v11-0005-meson-prereq-Refactor-PG_TEST_EXTRA-logic-in-aut.patch"
^ permalink raw reply [nested|flat] 52+ messages in thread
* [PATCH v12 04/15] meson: prereq: Add src/tools/gen_export.pl
@ 2022-01-20 07:36 Andres Freund <[email protected]>
0 siblings, 0 replies; 52+ messages in thread
From: Andres Freund @ 2022-01-20 07:36 UTC (permalink / raw)
Currently the logic is all in src/Makefile.shlib. This adds a sketch of a
generation script that can be used from meson.
Author: Andres Freund <[email protected]>
Reviewed-By: Peter Eisentraut <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/tools/gen_export.pl | 81 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 81 insertions(+)
create mode 100644 src/tools/gen_export.pl
diff --git a/src/tools/gen_export.pl b/src/tools/gen_export.pl
new file mode 100644
index 00000000000..3a368c85381
--- /dev/null
+++ b/src/tools/gen_export.pl
@@ -0,0 +1,81 @@
+use strict;
+use warnings;
+use Getopt::Long;
+
+my $format;
+my $libname;
+my $input;
+my $output;
+
+GetOptions(
+ 'format:s' => \$format,
+ 'libname:s' => \$libname,
+ 'input:s' => \$input,
+ 'output:s' => \$output) or die "wrong arguments";
+
+if (not ($format eq 'aix' or $format eq 'darwin' or $format eq 'gnu' or $format eq 'win'))
+{
+ die "$0: $format is not yet handled (only aix, darwin, gnu, win are)\n";
+}
+
+open(my $input_handle, '<', $input)
+ or die "$0: could not open input file '$input': $!\n";
+
+open(my $output_handle, '>', $output)
+ or die "$0: could not open output file '$output': $!\n";
+
+
+if ($format eq 'gnu')
+{
+ print $output_handle "{
+ global:
+";
+}
+elsif ($format eq 'win')
+{
+ # XXX: Looks like specifying LIBRARY $libname is optional, which makes it
+ # easier to build a generic command for generating export files...
+ if ($libname)
+ {
+ print $output_handle "LIBRARY $libname\n";
+ }
+ print $output_handle "EXPORTS\n";
+}
+
+while (<$input_handle>)
+{
+ if (/^#/)
+ {
+ # don't do anything with a comment
+ }
+ elsif (/^(\S+)\s+(\S+)/)
+ {
+ if ($format eq 'aix')
+ {
+ print $output_handle " $1\n";
+ }
+ elsif ($format eq 'darwin')
+ {
+ print $output_handle " _$1\n";
+ }
+ elsif ($format eq 'gnu')
+ {
+ print $output_handle " $1;\n";
+ }
+ elsif ($format eq 'win')
+ {
+ print $output_handle " $1 @ $2\n";
+ }
+ }
+ else
+ {
+ die "$0: unexpected line $_\n";
+ }
+}
+
+if ($format eq 'gnu')
+{
+ print $output_handle " local: *;
+};
+";
+}
--
2.37.0.3.g30cc8d0f14
--3jz64y4qgcz5d4ku
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v12-0005-meson-prereq-Refactor-PG_TEST_EXTRA-logic-in-aut.patch"
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2022-03-30 13:44 Justin Pryzby <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Justin Pryzby @ 2022-03-30 13:44 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]
On Tue, Mar 01, 2022 at 02:14:13PM +0900, Kyotaro Horiguchi wrote:
> Rebased on a recent xlog refactoring.
It'll come as no surprise that this neds to be rebased again.
At least a few typos I reported in January aren't fixed.
Set to "waiting".
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2022-03-31 04:58 Kyotaro Horiguchi <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2022-03-31 04:58 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]
Thanks! Version 20 is attached.
At Wed, 30 Mar 2022 08:44:02 -0500, Justin Pryzby <[email protected]> wrote in
> On Tue, Mar 01, 2022 at 02:14:13PM +0900, Kyotaro Horiguchi wrote:
> > Rebased on a recent xlog refactoring.
>
> It'll come as no surprise that this neds to be rebased again.
> At least a few typos I reported in January aren't fixed.
> Set to "waiting".
Oh, I'm sorry for overlooking it. It somehow didn't show up on my
mailer.
> I started looking at this and reviewed docs and comments again.
>
> > +typedef struct PendingCleanup
> > +{
> > + RelFileNode relnode; /* relation that may need to be deleted */
> > + int op; /* operation mask */
> > + bool bufpersistence; /* buffer persistence to set */
> > + int unlink_forknum; /* forknum to unlink */
>
> This can be of data type "ForkNumber"
Right. Fixed.
> > + * We are going to create an init fork. If server crashes before the
> > + * current transaction ends the init fork left alone corrupts data while
> > + * recovery. The mark file works as the sentinel to identify that
> > + * situation.
>
> s/while/during/
This was in v17, but dissapeared in v18.
> > + * index-init fork needs further initialization. ambuildempty shoud do
>
> should (I reported this before)
>
> > + if (inxact_created)
> > + {
> > + SMgrRelation srel = smgropen(rnode, InvalidBackendId);
> > +
> > + /*
> > + * INIT forks never be loaded to shared buffer so no point in dropping
>
> "are never loaded"
If was fixed in v18.
> > + elog(DEBUG1, "perform in-place persistnce change");
>
> persistence (I reported this before)
Sorry. Fixed.
> > + /*
> > + * While wal_level >= replica, switching to LOGGED requires the
> > + * relation content to be WAL-logged to recover the table.
> > + * We don't emit this fhile wal_level = minimal.
>
> while (or "if")
There are "While" and "fhile". I changed the latter to "if".
> > + * The relation is persistent and stays remain persistent. Don't
> > + * drop the buffers for this relation.
>
> "stays remain" is redundant (I reported this before)
Thanks. I changed it to "stays persistent".
> > + if (unlink(rm_path) < 0)
> > + ereport(ERROR,
> > + (errcode_for_file_access(),
> > + errmsg("could not remove file \"%s\": %m",
> > + rm_path)));
>
> The parens around errcode are unnecessary since last year.
> I suggest to avoid using them here and elsewhere.
It is just moved from elsewhere without editing, but of course I can
do that. I didn't know about that change of ereport and not found the
corresponding commit, but I found that Tom mentioned that change.
https://www.postgresql.org/message-id/flat/5063.1584641224%40sss.pgh.pa.us#63e611c30800133bbddb48de8...
> Now that we can rely on having varargs macros, I think we could
> stop requiring the extra level of parentheses, ie instead of
...
> ereport(ERROR,
> errcode(ERRCODE_DIVISION_BY_ZERO),
> errmsg("division by zero"));
>
> (The old syntax had better still work, of course. I'm not advocating
> running around and changing existing calls.)
I changed all ereport calls added by this patch to this style.
> > + * And we may have SMGR_MARK_UNCOMMITTED file. Remove it if the
> > + * fork files has been successfully removed. It's ok if the file
>
> file
Fixed.
> > + <para>
> > + All tables in the current database in a tablespace can be changed by using
>
> given tablespace
I did /database in a tablespace/database in the given tablespace/. Is
it right?
> > + the <literal>ALL IN TABLESPACE</literal> form, which will lock all tables
>
> which will first lock
>
> > + to be changed first and then change each one. This form also supports
>
> remove "first" here
This is almost a dead copy of the description of SET TABLESPACE. This
change makes the two almost the same description vary slightly in that
wordings. Anyway I did that as suggested only for the part this patch
adds in this version.
> > + <literal>OWNED BY</literal>, which will only change tables owned by the
> > + roles specified. If the <literal>NOWAIT</literal> option is specified
>
> specified roles.
> is specified, (comma)
This is the same as above. I did that but it makes the description
differ from another almost-the-same description.
> > + then the command will fail if it is unable to acquire all of the locks
>
> if it is unable to immediately acquire
>
> > + required immediately. The <literal>information_schema</literal>
>
> remove immediately
Ditto.
> > + relations are not considered part of the system catalogs and will be
>
> I think you need to first say that "relations in the pg_catalog schema cannot
> be changed".
Mmm. I don't agree on this. Aren't such "exceptions"-ish descriptions
usually placed after the descriptions of how the feature works? This
is also the same structure with SET TABLESPACE.
> in patch 2/2:
> typo: persistene
Hmm. Bad. I checked the spellings of the whole patches and found some
typos.
+ * The crashed trasaction did SET UNLOGGED. This relation
+ * is restored to a LOGGED relation.
s/trasaction/transaction/
+ errmsg("could not crete mark file \"%s\": %m", path));
s/crete/create/
Then rebased on 9c08aea6a3 then pgindent'ed.
Thanks!
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2022-03-31 05:37 Justin Pryzby <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Justin Pryzby @ 2022-03-31 05:37 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]
On Thu, Mar 31, 2022 at 01:58:45PM +0900, Kyotaro Horiguchi wrote:
> Thanks! Version 20 is attached.
The patch failed an all CI tasks, and seems to have caused the macos task to
hang.
http://cfbot.cputube.org/kyotaro-horiguchi.html
Would you send a fixed patch, or remove this thread from the CFBOT ? Otherwise
cirrrus will try to every day to rerun but take 1hr to time out, which is twice
as slow as the slowest OS.
I think this patch should be moved to the next CF and set to v16.
Thanks,
--
Justin
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2022-03-31 09:33 Kyotaro Horiguchi <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2022-03-31 09:33 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]
At Thu, 31 Mar 2022 00:37:07 -0500, Justin Pryzby <[email protected]> wrote in
> On Thu, Mar 31, 2022 at 01:58:45PM +0900, Kyotaro Horiguchi wrote:
> > Thanks! Version 20 is attached.
>
> The patch failed an all CI tasks, and seems to have caused the macos task to
> hang.
>
> http://cfbot.cputube.org/kyotaro-horiguchi.html
>
> Would you send a fixed patch, or remove this thread from the CFBOT ? Otherwis
e
> cirrrus will try to every day to rerun but take 1hr to time out, which is twice
> as slow as the slowest OS.
That is found to be a thinko that causes mark files left behind in new
database created in the logged version of CREATE DATABASE. It is
easily fixed.
That being said, this failure revealed that pg_checksums or
pg_basebackup dislikes the mark files. It happens even in a quite low
possibility. This would need further consideration and extra rounds of
reviews.
> I think this patch should be moved to the next CF and set to v16.
I don't think this can be commited to 15. So I post the fixed version
then move this to the next CF.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2022-03-31 09:36 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2022-03-31 09:36 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]
At Thu, 31 Mar 2022 18:33:18 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> I don't think this can be commited to 15. So I post the fixed version
> then move this to the next CF.
Then done. Thanks!
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2022-07-06 15:44 Jacob Champion <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Jacob Champion @ 2022-07-06 15:44 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: Justin Pryzby <[email protected]>; Tom Lane <[email protected]>; [email protected]; [email protected]; PostgreSQL Hackers <[email protected]>
On Thu, Mar 31, 2022 at 2:36 AM Kyotaro Horiguchi
<[email protected]> wrote:
>
> At Thu, 31 Mar 2022 18:33:18 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> > I don't think this can be commited to 15. So I post the fixed version
> > then move this to the next CF.
>
> Then done. Thanks!
Hello! This patchset will need to be rebased over latest -- looks like
b74e94dc27f (Rethink PROCSIGNAL_BARRIER_SMGRRELEASE) and 5c279a6d350
(Custom WAL Resource Managers) are interfering.
Thanks,
--Jacob
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2022-07-07 08:24 Kyotaro Horiguchi <[email protected]>
parent: Jacob Champion <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2022-07-07 08:24 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Wed, 6 Jul 2022 08:44:18 -0700, Jacob Champion <[email protected]> wrote in
> On Thu, Mar 31, 2022 at 2:36 AM Kyotaro Horiguchi
> <[email protected]> wrote:
> >
> > At Thu, 31 Mar 2022 18:33:18 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> > > I don't think this can be commited to 15. So I post the fixed version
> > > then move this to the next CF.
> >
> > Then done. Thanks!
>
> Hello! This patchset will need to be rebased over latest -- looks like
> b74e94dc27f (Rethink PROCSIGNAL_BARRIER_SMGRRELEASE) and 5c279a6d350
> (Custom WAL Resource Managers) are interfering.
Thank you for checking that! It got a wider attack by b0a55e4329
(RelFileNumber). The commit message suggests "relfilenode" as files
should be replaced with "relation storage/file" so I did that in
ResetUnloggedRelationsInDbspaceDir.
This patch said that:
> * INIT forks are never loaded to shared buffer so no point in
> * dropping buffers for such files.
But actually some *buildempty() functions use ReadBufferExtended() for
INIT_FORK. So that's wrong. So, I did that but... I don't like that.
Or I don't like that some AMs leave buffers for INIT fork after. But I
feel I'm misunderstanding here since I don't understand how the INIT
fork can work as expected after a crash that happens before the next
checkpoint flushes the buffers.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2022-07-19 04:33 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2022-07-19 04:33 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
(Mmm. I haven't noticed an annoying misspelling in the subejct X( )
> Thank you for checking that! It got a wider attack by b0a55e4329
> (RelFileNumber). The commit message suggests "relfilenode" as files
Then, now I stepped on my own foot. Rebased also on nodefuncs
autogeneration.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2022-09-28 08:21 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2022-09-28 08:21 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Just rebased.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2022-11-04 00:32 Ian Lawrence Barwick <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Ian Lawrence Barwick @ 2022-11-04 00:32 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
2022年9月28日(水) 17:21 Kyotaro Horiguchi <[email protected]>:
>
> Just rebased.
Hi
cfbot reports the patch no longer applies. As CommitFest 2022-11 is
currently underway, this would be an excellent time to update the patch.
Thanks
Ian Barwick
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2022-11-08 02:33 Kyotaro Horiguchi <[email protected]>
parent: Ian Lawrence Barwick <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Kyotaro Horiguchi @ 2022-11-08 02:33 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Fri, 4 Nov 2022 09:32:52 +0900, Ian Lawrence Barwick <[email protected]> wrote in
> cfbot reports the patch no longer applies. As CommitFest 2022-11 is
> currently underway, this would be an excellent time to update the patch.
Indeed, thanks! I'll do that in a few days.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2022-11-18 08:25 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 52+ messages in thread
From: Kyotaro Horiguchi @ 2022-11-18 08:25 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Tue, 08 Nov 2022 11:33:53 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> Indeed, thanks! I'll do that in a few days.
Got too late, but rebased.. The contents of the two patches in the
last version was a bit shuffled but they are fixed.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2024-01-22 04:36 Peter Smith <[email protected]>
0 siblings, 1 reply; 52+ messages in thread
From: Peter Smith @ 2024-01-22 04:36 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
2024-01 Commitfest.
Hi, This patch has a CF status of "Needs Review" [1], but it seems
there was a CFbot test failure last time it was run [2]. Please have a
look and post an updated version if necessary.
======
[1] https://commitfest.postgresql.org/46/3461/
[2] https://cirrus-ci.com/task/6050020441456640
Kind Regards,
Peter Smith.
^ permalink raw reply [nested|flat] 52+ messages in thread
* Re: In-placre persistance change of a relation
@ 2024-01-23 04:15 Kyotaro Horiguchi <[email protected]>
parent: Peter Smith <[email protected]>
0 siblings, 0 replies; 52+ messages in thread
From: Kyotaro Horiguchi @ 2024-01-23 04:15 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Mon, 22 Jan 2024 15:36:31 +1100, Peter Smith <[email protected]> wrote in
> 2024-01 Commitfest.
>
> Hi, This patch has a CF status of "Needs Review" [1], but it seems
> there was a CFbot test failure last time it was run [2]. Please have a
> look and post an updated version if necessary.
Thanks! I have added the necessary includes to the header file this
patch adds. With this change, "make headerscheck" now passes. However,
when I run "make cpluspluscheck" in my environment, it generates a
large number of errors in other areas, but I didn't find one related
to this patch.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 52+ messages in thread
end of thread, other threads:[~2024-01-23 04:15 UTC | newest]
Thread overview: 52+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-11-11 08:33 In-placre persistance change of a relation Kyotaro Horiguchi <[email protected]>
2020-11-11 22:18 ` Andres Freund <[email protected]>
2020-11-12 06:55 ` Kyotaro Horiguchi <[email protected]>
2020-11-13 04:22 ` Kyotaro Horiguchi <[email protected]>
2020-11-13 06:43 ` [email protected] <[email protected]>
2020-11-13 07:15 ` [email protected] <[email protected]>
2020-11-13 08:23 ` Kyotaro Horiguchi <[email protected]>
2020-12-04 07:49 ` [email protected] <[email protected]>
2020-12-24 08:02 ` Kyotaro Horiguchi <[email protected]>
2020-12-25 00:12 ` Kyotaro Horiguchi <[email protected]>
2021-01-08 05:47 ` Kyotaro Horiguchi <[email protected]>
2021-01-08 08:52 ` Kyotaro Horiguchi <[email protected]>
2021-01-12 09:58 ` Kyotaro Horiguchi <[email protected]>
2021-01-14 08:32 ` Kyotaro Horiguchi <[email protected]>
2021-03-25 05:08 ` Kyotaro Horiguchi <[email protected]>
2021-03-25 05:15 ` Kyotaro Horiguchi <[email protected]>
2021-12-17 09:10 ` Jakub Wartak <[email protected]>
2021-12-17 12:46 ` Justin Pryzby <[email protected]>
2021-12-17 14:33 ` Jakub Wartak <[email protected]>
2021-12-17 19:10 ` Justin Pryzby <[email protected]>
2021-12-20 06:28 ` Kyotaro Horiguchi <[email protected]>
2021-12-20 07:53 ` Kyotaro Horiguchi <[email protected]>
2021-12-20 07:59 ` Jakub Wartak <[email protected]>
2021-12-20 08:39 ` Kyotaro Horiguchi <[email protected]>
2021-12-20 08:52 ` Kyotaro Horiguchi <[email protected]>
2021-12-20 13:38 ` Jakub Wartak <[email protected]>
2021-12-21 08:13 ` Kyotaro Horiguchi <[email protected]>
2021-12-21 11:04 ` Kyotaro Horiguchi <[email protected]>
2021-12-21 13:07 ` Jakub Wartak <[email protected]>
2021-12-22 06:13 ` Kyotaro Horiguchi <[email protected]>
2021-12-22 08:42 ` Jakub Wartak <[email protected]>
2021-12-23 06:01 ` Kyotaro Horiguchi <[email protected]>
2021-12-23 06:33 ` Kyotaro Horiguchi <[email protected]>
2020-11-13 07:47 ` Kyotaro Horiguchi <[email protected]>
2022-01-20 07:36 [PATCH v13 05/20] meson: prereq: Add src/tools/gen_export.pl Andres Freund <[email protected]>
2022-01-20 07:36 [PATCH v12 04/15] meson: prereq: Add src/tools/gen_export.pl Andres Freund <[email protected]>
2022-01-20 07:36 [PATCH v13 05/20] meson: prereq: Add src/tools/gen_export.pl Andres Freund <[email protected]>
2022-01-20 07:36 [PATCH v11 4/9] meson: prereq: Add src/tools/gen_export.pl Andres Freund <[email protected]>
2022-03-30 13:44 Re: In-placre persistance change of a relation Justin Pryzby <[email protected]>
2022-03-31 04:58 ` Kyotaro Horiguchi <[email protected]>
2022-03-31 05:37 ` Justin Pryzby <[email protected]>
2022-03-31 09:33 ` Kyotaro Horiguchi <[email protected]>
2022-03-31 09:36 ` Kyotaro Horiguchi <[email protected]>
2022-07-06 15:44 ` Jacob Champion <[email protected]>
2022-07-07 08:24 ` Kyotaro Horiguchi <[email protected]>
2022-07-19 04:33 ` Kyotaro Horiguchi <[email protected]>
2022-09-28 08:21 ` Kyotaro Horiguchi <[email protected]>
2022-11-04 00:32 ` Ian Lawrence Barwick <[email protected]>
2022-11-08 02:33 ` Kyotaro Horiguchi <[email protected]>
2022-11-18 08:25 ` Kyotaro Horiguchi <[email protected]>
2024-01-22 04:36 Re: In-placre persistance change of a relation Peter Smith <[email protected]>
2024-01-23 04:15 ` Kyotaro Horiguchi <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox